Skip to content

Fix SDL event initialization and backend context allocation#30

Merged
kimkulling merged 2 commits intomainfrom
bugfix/fix_undefined_behavior
Apr 12, 2026
Merged

Fix SDL event initialization and backend context allocation#30
kimkulling merged 2 commits intomainfrom
bugfix/fix_undefined_behavior

Conversation

@kimkulling
Copy link
Copy Markdown
Owner

@kimkulling kimkulling commented Apr 12, 2026

  • This initialization is based on an union and will cause undefined behavior.

Summary by CodeRabbit

  • Bug Fixes
    • Improved renderer event handling to reduce rare event-processing issues and enhance runtime stability.

- This initialization is based on an union and will cause undefined behavior.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 12, 2026

Walkthrough

Single change in Renderer::update(const Context &ctx): SDL event local variable initialization changed from value-initialized (SDL_Event event = {};) to default-uninitialized (SDL_Event event;), altering whether event fields are zeroed before being used.

Changes

Cohort / File(s) Summary
SDL2 Renderer Backend
src/backends/sdl2_renderer.cpp
Replaced value-initialized SDL_Event event = {} with default-uninitialized SDL_Event event; in Renderer::update(const Context &ctx), affecting initial contents of the event struct passed to IODevice::update(event).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title mentions both SDL event initialization and backend context allocation, but the actual changes only affect SDL event initialization (removing value-initialization). The title is broader than the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bugfix/fix_undefined_behavior

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/backends/sdl2_renderer.cpp`:
- Line 282: The line assigning ctx.mBackendCtx = new BackendContext;. has an
accidental trailing '.' after the semicolon which causes a compile error; edit
the assignment where ctx.mBackendCtx is set (new BackendContext) to remove the
extra '.' so the statement ends with just the semicolon and rebuild.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a6897168-df2b-4695-8979-09776e8b2aa1

📥 Commits

Reviewing files that changed from the base of the PR and between 626deb3 and 4ad7714.

📒 Files selected for processing (1)
  • src/backends/sdl2_renderer.cpp

Comment thread src/backends/sdl2_renderer.cpp Outdated
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/backends/sdl2_renderer.cpp (2)

504-504: ⚠️ Potential issue | 🔴 Critical

Syntax error: int32_t int s is invalid.

This line has an erroneous int keyword that will cause a compilation failure on big-endian systems.

🐛 Proposed fix
-    const int32_t int s = (bytesPerPixel == 4) ? 0 : 8;
+    const int32_t s = (bytesPerPixel == 4) ? 0 : 8;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backends/sdl2_renderer.cpp` at line 504, The line declaring s is
malformed ("int32_t int s") causing a compile error; remove the stray "int" and
declare s correctly as a single int32_t initialized from bytesPerPixel (e.g.
replace the broken declaration in src/backends/sdl2_renderer.cpp with a single
declaration of the symbol s using int32_t and the existing ternary using
bytesPerPixel) so the variable s is properly defined on all platforms.

472-476: ⚠️ Potential issue | 🟠 Major

Bug: Accessing wrong union member for SDL_MOUSEMOTION events.

For SDL_MOUSEMOTION events, the active union member is event.motion, not event.button. Accessing event.button.x/y instead of event.motion.x/y is technically undefined behavior (accessing inactive union member).

🐛 Proposed fix
             case SDL_MOUSEMOTION:
                 {
-                    const int32_t x = event.button.x;
-                    const int32_t y = event.button.y;
+                    const int32_t x = event.motion.x;
+                    const int32_t y = event.motion.y;
                 } break;

Note: The variables x and y are also currently unused. Consider either removing this case or implementing the mouse motion handling.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backends/sdl2_renderer.cpp` around lines 472 - 476, The SDL_MOUSEMOTION
case is reading the wrong union member (event.button) which is undefined; change
accesses to use event.motion.x and event.motion.y in the SDL_MOUSEMOTION branch
(or remove the unused x/y locals if motion handling is not needed). Locate the
SDL_MOUSEMOTION switch case in sdl2_renderer.cpp and update the variables (or
drop the case) so the active union member event.motion is used and unused
variables are eliminated or used for actual mouse-move handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/backends/sdl2_renderer.cpp`:
- Line 504: The line declaring s is malformed ("int32_t int s") causing a
compile error; remove the stray "int" and declare s correctly as a single
int32_t initialized from bytesPerPixel (e.g. replace the broken declaration in
src/backends/sdl2_renderer.cpp with a single declaration of the symbol s using
int32_t and the existing ternary using bytesPerPixel) so the variable s is
properly defined on all platforms.
- Around line 472-476: The SDL_MOUSEMOTION case is reading the wrong union
member (event.button) which is undefined; change accesses to use event.motion.x
and event.motion.y in the SDL_MOUSEMOTION branch (or remove the unused x/y
locals if motion handling is not needed). Locate the SDL_MOUSEMOTION switch case
in sdl2_renderer.cpp and update the variables (or drop the case) so the active
union member event.motion is used and unused variables are eliminated or used
for actual mouse-move handling.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 21f1021d-929f-4d39-b122-590ac6ef162f

📥 Commits

Reviewing files that changed from the base of the PR and between 4ad7714 and af08ad9.

📒 Files selected for processing (1)
  • src/backends/sdl2_renderer.cpp

@kimkulling kimkulling merged commit 9d6dc59 into main Apr 12, 2026
4 checks passed
@kimkulling kimkulling deleted the bugfix/fix_undefined_behavior branch April 12, 2026 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant