Merged
Conversation
WalkthroughReplaces direct SDLContext usage with an indirection BackendContext stored on Context (mBackendCtx). Adds inline getBackendContext helper and Renderer::getSurfaceInfo. Refactors SDL backend, tinyui core, and widgets to use the backend handle; adjusts renderer initialization and rendering calls. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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 (1)
268-284:⚠️ Potential issue | 🔴 CriticalMemory leak:
SDLContextallocated before error checks can trigger early returns.
SDLContext::create()allocates memory at line 268, but the early return at line 271 (if window already created) leaks this allocation. Additionally, afterBackendContextis allocated at line 274, all subsequent error returns (lines 279, 284, 296, 302, 309, 321) leak bothsdlCtxandctx.mBackendCtx.🐛 Proposed fix: Cleanup on error paths
ret_code Renderer::initScreen(Context &ctx, int32_t x, int32_t y, int32_t w, int32_t h) { if (!ctx.mCreated) { ctx.mLogger(LogSeverity::Error, "Not initialzed."); return ErrorCode; } + + // Allocate context wrapper first + ctx.mBackendCtx = new BackendContext; + SDLContext *sdlCtx = SDLContext::create(); + ctx.mBackendCtx->mHandle = static_cast<void*>(sdlCtx); + if (sdlCtx->mWindow != nullptr) { ctx.mLogger(LogSeverity::Error, "Already created."); + delete ctx.mBackendCtx; + ctx.mBackendCtx = nullptr; + delete sdlCtx; return ErrorCode; } - - ctx.mBackendCtx = new BackendContext; - ctx.mBackendCtx->mHandle = (void*) sdlCtx; if (TTF_Init() == -1) { ctx.mLogger(LogSeverity::Error, "TTF init failed."); + sdlCtx->destroy(); + delete ctx.mBackendCtx; + ctx.mBackendCtx = nullptr; return ErrorCode; } + // ... similar cleanup for other error pathsConsider using RAII or a cleanup helper to avoid repetitive error 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 268 - 284, SDLContext is allocated via SDLContext::create() into sdlCtx before multiple early-return error checks, causing leaks; ensure you free/cleanup sdlCtx and any allocated ctx.mBackendCtx on every error path (after the SDLContext::create() call and after ctx.mBackendCtx = new BackendContext) — either convert sdlCtx and ctx.mBackendCtx to RAII/smart pointers (unique_ptr) or add a cleanup helper that deletes the SDLContext and BackendContext and resets ctx.mBackendCtx before each return from the error checks around TTF_Init(), loadDefaultFont(), and subsequent failures; update the error-return branches to call that cleanup (or rely on destructor of smart pointers) so no allocated objects are leaked.src/tinyui.cpp (1)
191-191:⚠️ Potential issue | 🔴 CriticalCompilation error:
std::strlenrequires<cstring>header.The pipeline failure indicates that
std::strlenis not recognized. The<cstring>header was removed from this file but is still needed for line 191.🐛 Proposed fix: Add missing include
`#include` "backends/sdl2_iodevice.h" `#include` <iostream> +#include <cstring> namespace tinyui {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tinyui.cpp` at line 191, The build fails because std::strlen is used in the conditional with strncmp (see the comparison using ctx.mStyle.mFont.mName and defaultFont) but <cstring> is not included; fix this by adding the missing `#include` <cstring> to the top of src/tinyui.cpp (ensure it is included once alongside the other standard headers) so that std::strlen and related C-string functions are available for the strncmp/std::strlen usage.
🧹 Nitpick comments (3)
src/widgets.h (1)
314-320:beginChild()andendChild()are declared but their implementations are empty stubs.Looking at the implementations in
src/widgets.cpp(lines 756-764), these methods only retrieve the context and immediately returntruewithout performing any actual child window operations. Consider either:
- Adding TODO comments documenting the intended functionality
- Deferring the declaration until the implementation is ready
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.h` around lines 314 - 320, The declarations for beginChild() and endChild() correspond to empty stub implementations (beginChild and endChild) that simply return true; either add clear TODO comments in both the header and their implementations documenting the intended behavior and any required context setup/teardown for child windows (so future implementers know what's missing), or remove/defer the declarations until full implementations exist; update the function doc comments above beginChild and endChild to reflect the TODO or the reason for deferral so the API surface is not misleading.src/widgets.cpp (1)
756-764: Stub implementations fetch context but don't use it.Both
beginChild()andendChild()retrieve the context but immediately returntruewithout any logic. The unused variable will generate compiler warnings.♻️ Suppress warnings until implementation is ready
bool Widgets::beginChild() { - auto &ctx = TinyUi::getContext(); + [[maybe_unused]] auto &ctx = TinyUi::getContext(); + // TODO: Implement child window creation return true; } bool Widgets::endChild() { - auto &ctx = TinyUi::getContext(); + [[maybe_unused]] auto &ctx = TinyUi::getContext(); + // TODO: Implement child window closing return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.cpp` around lines 756 - 764, The functions Widgets::beginChild and Widgets::endChild currently call TinyUi::getContext() but don't use the returned ctx, causing compiler warnings; to suppress warnings until real logic is added, either remove the unused call or explicitly mark the variable unused (e.g., add a single "(void)ctx;" after the auto &ctx = TinyUi::getContext() line) in both Widgets::beginChild and Widgets::endChild so the compiler knows the variable is intentionally unused while preserving the future call site.src/tinyui.h (1)
384-386: Consider adding a validity check or initialized flag toBackendContext.The
BackendContextwrapper only contains avoid* mHandle. SincemHandlecan benullptreven whenBackendContextis allocated (as seen in init error paths), callers cannot easily distinguish between "not initialized" and "partially initialized" states.♻️ Optional: Add an initialized flag
struct BackendContext { void *mHandle{nullptr}; + bool mInitialized{false}; ///< True when backend is fully initialized };This would allow guards like
if (!ctx.mBackendCtx || !ctx.mBackendCtx->mInitialized)for safer checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tinyui.h` around lines 384 - 386, Add a boolean initialized flag to BackendContext (e.g., bool mInitialized) and ensure it is false by default; set it to true only after the backend handle (mHandle) is successfully created in the init code path and reset to false in failure/cleanup paths. Update any code that inspects BackendContext (calls that currently test mHandle) to also check the new mInitialized flag (or provide an isValid() accessor on BackendContext) so callers can distinguish "allocated but not initialized" from "initialized" states. Ensure constructors/destructors and error paths that null out mHandle also update mInitialized consistently.
🤖 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`:
- Around line 407-419: closeScreen currently only destroys mWindow and quits TTF
but leaks the SDL renderer and heap contexts; update Renderer::closeScreen to
call SDLContext::destroy() (which should handle destroying mRenderer and any SDL
subresources), then free/delete the SDLContext instance and the BackendContext
wrapper retrieved via getBackendContext(ctx), and finally set ctx.mBackendCtx to
nullptr; ensure you still call TTF_Quit() and set sdlCtx->mWindow = nullptr (or
rely on SDLContext::destroy() to clear members) but do not return before
cleaning up the BackendContext object.
- Around line 340-347: In initScreen overload, getBackendContext(ctx) is invoked
before ctx.mBackendCtx is allocated, causing a null deref; allocate and
initialize the backend context (create a new SDLContext and assign it to
ctx.mBackendCtx or call the existing backend-context factory) before calling
getBackendContext(ctx), then proceed to set sdlCtx->mRenderer, sdlCtx->mWindow,
call showDriverInUse(ctx), set sdlCtx->mSurface and sdlCtx->mOwner
appropriately; ensure creation path mirrors other initScreen variants so
SDLContext is fully initialized before use.
- Around line 516-526: In getSurfaceInfo, you are incorrectly casting
ctx.mBackendCtx to SDLContext*; instead retrieve the BackendContext first and
cast its mHandle to SDLContext* (same pattern as getBackendContext()), check
that the BackendContext and its mHandle/mSurface are non-null before accessing
mSurface, and return ErrorCode on nulls; update references to use the
SDLContext* derived from ctx.mBackendCtx->mHandle.
In `@src/backends/sdl2_renderer.h`:
- Around line 103-106: getBackendContext currently uses a C-style cast and
dereferences ctx.mBackendCtx and its mHandle without null checks; change it to
verify ctx.mBackendCtx and ctx.mBackendCtx->mHandle are non-null and return
nullptr if either is null, and replace the C-style cast with a
static_cast<SDLContext*> when converting mHandle to SDLContext*; update callers
(e.g., places that assume getBackendContext() always returns valid) to handle a
nullptr return before dereferencing.
In `@src/widgets.cpp`:
- Around line 224-226: The nullptr check for ctx.mBackendCtx is insufficient
because mBackendCtx can exist while its SDLContext::mRenderer is still nullptr;
update the guard in the function that currently checks ctx.mBackendCtx (use the
same context where InvalidRenderHandle is returned) to also verify the
underlying renderer is initialized (e.g., ensure ctx.mSDLContext and
ctx.mSDLContext->mRenderer are non-null) or call an isInitialized()/isReady()
helper on ctx.mBackendCtx if available; change the single-condition return into
a combined check so rendering code never dereferences a nullptr mRenderer.
---
Outside diff comments:
In `@src/backends/sdl2_renderer.cpp`:
- Around line 268-284: SDLContext is allocated via SDLContext::create() into
sdlCtx before multiple early-return error checks, causing leaks; ensure you
free/cleanup sdlCtx and any allocated ctx.mBackendCtx on every error path (after
the SDLContext::create() call and after ctx.mBackendCtx = new BackendContext) —
either convert sdlCtx and ctx.mBackendCtx to RAII/smart pointers (unique_ptr) or
add a cleanup helper that deletes the SDLContext and BackendContext and resets
ctx.mBackendCtx before each return from the error checks around TTF_Init(),
loadDefaultFont(), and subsequent failures; update the error-return branches to
call that cleanup (or rely on destructor of smart pointers) so no allocated
objects are leaked.
In `@src/tinyui.cpp`:
- Line 191: The build fails because std::strlen is used in the conditional with
strncmp (see the comparison using ctx.mStyle.mFont.mName and defaultFont) but
<cstring> is not included; fix this by adding the missing `#include` <cstring> to
the top of src/tinyui.cpp (ensure it is included once alongside the other
standard headers) so that std::strlen and related C-string functions are
available for the strncmp/std::strlen usage.
---
Nitpick comments:
In `@src/tinyui.h`:
- Around line 384-386: Add a boolean initialized flag to BackendContext (e.g.,
bool mInitialized) and ensure it is false by default; set it to true only after
the backend handle (mHandle) is successfully created in the init code path and
reset to false in failure/cleanup paths. Update any code that inspects
BackendContext (calls that currently test mHandle) to also check the new
mInitialized flag (or provide an isValid() accessor on BackendContext) so
callers can distinguish "allocated but not initialized" from "initialized"
states. Ensure constructors/destructors and error paths that null out mHandle
also update mInitialized consistently.
In `@src/widgets.cpp`:
- Around line 756-764: The functions Widgets::beginChild and Widgets::endChild
currently call TinyUi::getContext() but don't use the returned ctx, causing
compiler warnings; to suppress warnings until real logic is added, either remove
the unused call or explicitly mark the variable unused (e.g., add a single
"(void)ctx;" after the auto &ctx = TinyUi::getContext() line) in both
Widgets::beginChild and Widgets::endChild so the compiler knows the variable is
intentionally unused while preserving the future call site.
In `@src/widgets.h`:
- Around line 314-320: The declarations for beginChild() and endChild()
correspond to empty stub implementations (beginChild and endChild) that simply
return true; either add clear TODO comments in both the header and their
implementations documenting the intended behavior and any required context
setup/teardown for child windows (so future implementers know what's missing),
or remove/defer the declarations until full implementations exist; update the
function doc comments above beginChild and endChild to reflect the TODO or the
reason for deferral so the API surface is not misleading.
🪄 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: 744c8e5b-f5ea-4e81-b64c-5e3c69d62c84
📒 Files selected for processing (6)
src/backends/sdl2_renderer.cppsrc/backends/sdl2_renderer.hsrc/tinyui.cppsrc/tinyui.hsrc/widgets.cppsrc/widgets.h
Comment on lines
407
to
419
| ret_code Renderer::closeScreen(Context &ctx) { | ||
| if (ctx.mSDLContext->mWindow == nullptr) { | ||
| SDLContext *sdlCtx = getBackendContext(ctx); | ||
| if (sdlCtx->mWindow == nullptr) { | ||
| return ErrorCode; | ||
| } | ||
|
|
||
| TTF_Quit(); | ||
|
|
||
| SDL_DestroyWindow(ctx.mSDLContext->mWindow); | ||
| ctx.mSDLContext->mWindow = nullptr; | ||
| SDL_DestroyWindow(sdlCtx->mWindow); | ||
| sdlCtx->mWindow = nullptr; | ||
|
|
||
| return ResultOk; | ||
| } |
There was a problem hiding this comment.
Resource leak: closeScreen does not clean up SDLContext or BackendContext.
The function destroys mWindow but does not:
- Destroy the SDL renderer (
mRenderer) - Free the
SDLContextheap object - Free the
BackendContextwrapper - Set
ctx.mBackendCtxtonullptr
As noted in the context snippets, SDLContext::destroy() exists but is never called.
🛡️ Proposed fix: Complete cleanup
ret_code Renderer::closeScreen(Context &ctx) {
SDLContext *sdlCtx = getBackendContext(ctx);
+ if (sdlCtx == nullptr) {
+ return ErrorCode;
+ }
if (sdlCtx->mWindow == nullptr) {
return ErrorCode;
}
TTF_Quit();
- SDL_DestroyWindow(sdlCtx->mWindow);
- sdlCtx->mWindow = nullptr;
+ sdlCtx->mOwner = true; // Take ownership for cleanup
+ sdlCtx->destroy(); // Destroys renderer, window, and frees SDLContext
+
+ delete ctx.mBackendCtx;
+ ctx.mBackendCtx = nullptr;
return ResultOk;
}🤖 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 407 - 419, closeScreen currently
only destroys mWindow and quits TTF but leaks the SDL renderer and heap
contexts; update Renderer::closeScreen to call SDLContext::destroy() (which
should handle destroying mRenderer and any SDL subresources), then free/delete
the SDLContext instance and the BackendContext wrapper retrieved via
getBackendContext(ctx), and finally set ctx.mBackendCtx to nullptr; ensure you
still call TTF_Quit() and set sdlCtx->mWindow = nullptr (or rely on
SDLContext::destroy() to clear members) but do not return before cleaning up the
BackendContext object.
Comment on lines
+516
to
+526
| ret_code Renderer::getSurfaceInfo(Context &ctx, int32_t &w, int32_t &h) { | ||
| SDLContext *sdlCtx = (SDLContext *) ctx.mBackendCtx; | ||
| if (sdlCtx->mSurface == nullptr) { | ||
| return ErrorCode; | ||
| } | ||
|
|
||
| w = sdlCtx->mSurface->w; | ||
| h = sdlCtx->mSurface->h; | ||
|
|
||
| return ResultOk; | ||
| } |
There was a problem hiding this comment.
Critical bug: Incorrect cast in getSurfaceInfo — casts mBackendCtx instead of mBackendCtx->mHandle.
Line 517 casts ctx.mBackendCtx directly to SDLContext*, but mBackendCtx is a BackendContext*, not the SDL handle. The correct cast should be on mBackendCtx->mHandle, as done in getBackendContext().
This will cause undefined behavior when accessing sdlCtx->mSurface.
🐛 Proposed fix
ret_code Renderer::getSurfaceInfo(Context &ctx, int32_t &w, int32_t &h) {
- SDLContext *sdlCtx = (SDLContext *) ctx.mBackendCtx;
+ if (ctx.mBackendCtx == nullptr) {
+ return ErrorCode;
+ }
+ const auto *sdlCtx = static_cast<const SDLContext*>(ctx.mBackendCtx->mHandle);
if (sdlCtx->mSurface == nullptr) {
return ErrorCode;
}🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 517-517: Replace the redundant type with "auto".
[warning] 517-517: Make the type of this variable a pointer-to-const. The current type of "sdlCtx" is "struct tinyui::SDLContext *".
🤖 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 516 - 526, In getSurfaceInfo,
you are incorrectly casting ctx.mBackendCtx to SDLContext*; instead retrieve the
BackendContext first and cast its mHandle to SDLContext* (same pattern as
getBackendContext()), check that the BackendContext and its mHandle/mSurface are
non-null before accessing mSurface, and return ErrorCode on nulls; update
references to use the SDLContext* derived from ctx.mBackendCtx->mHandle.
Comment on lines
+103
to
+106
| inline SDLContext *getBackendContext(Context &ctx) { | ||
| SDLContext *sdlCtx = (SDLContext*) ctx.mBackendCtx->mHandle; | ||
| return sdlCtx; | ||
| } |
There was a problem hiding this comment.
getBackendContext() lacks null-safety and uses C-style cast.
This helper dereferences ctx.mBackendCtx->mHandle without checking if either pointer is null. As shown in the context snippets, initScreen can fail after allocating mBackendCtx but before fully initializing it, leaving mHandle valid but pointing to an incompletely initialized SDLContext.
Additionally, prefer static_cast over C-style casts per C++ best practices.
🛡️ Proposed fix with null checks and proper cast
inline SDLContext *getBackendContext(Context &ctx) {
- SDLContext *sdlCtx = (SDLContext*) ctx.mBackendCtx->mHandle;
- return sdlCtx;
+ if (ctx.mBackendCtx == nullptr) {
+ return nullptr;
+ }
+ return static_cast<SDLContext*>(ctx.mBackendCtx->mHandle);
}Note: All callers of getBackendContext() should then check for a null return value before dereferencing.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 104-104: Replace the redundant type with "auto".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backends/sdl2_renderer.h` around lines 103 - 106, getBackendContext
currently uses a C-style cast and dereferences ctx.mBackendCtx and its mHandle
without null checks; change it to verify ctx.mBackendCtx and
ctx.mBackendCtx->mHandle are non-null and return nullptr if either is null, and
replace the C-style cast with a static_cast<SDLContext*> when converting mHandle
to SDLContext*; update callers (e.g., places that assume getBackendContext()
always returns valid) to handle a nullptr return before dereferencing.
Comment on lines
+224
to
226
| if (ctx.mBackendCtx == nullptr) { | ||
| return InvalidRenderHandle; | ||
| } |
There was a problem hiding this comment.
Null check is insufficient: mBackendCtx != nullptr does not guarantee renderer is initialized.
Per the context snippets, initScreen allocates mBackendCtx early (line 274 in sdl2_renderer.cpp) but can fail on subsequent steps before mRenderer is assigned (line 305). The previous check ctx.mSDLContext->mRenderer == nullptr directly verified the renderer existed; this new check only verifies the wrapper exists.
This creates a window where mBackendCtx != nullptr but the underlying SDLContext::mRenderer is still nullptr, leading to crashes when rendering functions dereference it.
🛡️ Proposed fix: Strengthen the guard
Consider checking both the backend context and that the handle/renderer are properly initialized:
ret_code Widgets::label(Id id, Id parentId, const char *text, const Rect &rect, Alignment alignment) {
auto &ctx = TinyUi::getContext();
- if (ctx.mBackendCtx == nullptr) {
+ if (ctx.mBackendCtx == nullptr || ctx.mBackendCtx->mHandle == nullptr) {
return InvalidRenderHandle;
}Alternatively, expose an isInitialized() helper that validates the full initialization state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/widgets.cpp` around lines 224 - 226, The nullptr check for
ctx.mBackendCtx is insufficient because mBackendCtx can exist while its
SDLContext::mRenderer is still nullptr; update the guard in the function that
currently checks ctx.mBackendCtx (use the same context where InvalidRenderHandle
is returned) to also verify the underlying renderer is initialized (e.g., ensure
ctx.mSDLContext and ctx.mSDLContext->mRenderer are non-null) or call an
isInitialized()/isReady() helper on ctx.mBackendCtx if available; change the
single-condition return into a combined check so rendering code never
dereferences a nullptr mRenderer.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/backends/sdl2_renderer.cpp (3)
341-343:⚠️ Potential issue | 🔴 CriticalInitialize backend context before
getBackendContext(ctx)in external init overload.On Line 341,
getBackendContext(ctx)dereferencesctx.mBackendCtx->mHandle, but this overload does not initializectx.mBackendCtxfirst. This is a null-dereference path.🔧 Proposed fix
ret_code Renderer::initScreen(Context &ctx, SDL_Window *window, SDL_Renderer *renderer) { @@ TTF_Init(); - SDLContext *sdlCtx = getBackendContext(ctx); + if (ctx.mBackendCtx == nullptr) { + ctx.mBackendCtx = new BackendContext; + ctx.mBackendCtx->mHandle = static_cast<void *>(SDLContext::create()); + } + SDLContext *sdlCtx = getBackendContext(ctx); + if (sdlCtx == nullptr) { + return ErrorCode; + } sdlCtx->mRenderer = renderer; sdlCtx->mWindow = window;As per coding guidelines, "Validate inputs at function start".
🤖 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 341 - 343, The code calls getBackendContext(ctx) which dereferences ctx.mBackendCtx->mHandle before ctx.mBackendCtx is initialized; ensure the backend context is created/validated at the start of this external init overload: check that ctx.mBackendCtx is non-null (or allocate/assign a new backend context whose mHandle is properly initialized) before calling getBackendContext(ctx), then assign sdlCtx->mRenderer = renderer and sdlCtx->mWindow = window; alternatively call the existing initializer function that sets up ctx.mBackendCtx first (referencing getBackendContext, SDLContext, ctx.mBackendCtx, and mHandle).
517-524:⚠️ Potential issue | 🔴 CriticalFix invalid backend cast in
getSurfaceInfo.On Line 518,
ctx.mBackendCtx(typeBackendContext*) is cast directly toSDLContext*. That is undefined behavior. The SDL context is stored inctx.mBackendCtx->mHandle.🔧 Proposed fix
ret_code Renderer::getSurfaceInfo(Context &ctx, int32_t &w, int32_t &h) { - SDLContext *sdlCtx = (SDLContext *) ctx.mBackendCtx; - if (sdlCtx->mSurface == nullptr) { + if (ctx.mBackendCtx == nullptr || ctx.mBackendCtx->mHandle == nullptr) { + return ErrorCode; + } + auto *sdlCtx = static_cast<SDLContext *>(ctx.mBackendCtx->mHandle); + if (sdlCtx->mSurface == nullptr) { return ErrorCode; }🤖 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 517 - 524, Renderer::getSurfaceInfo currently casts ctx.mBackendCtx directly to SDLContext* which is undefined; instead obtain the BackendContext pointer from ctx.mBackendCtx, check it for null, then cast its mHandle member to SDLContext* (e.g., SDLContext *sdlCtx = static_cast<SDLContext*>(backendCtx->mHandle)), validate sdlCtx and sdlCtx->mSurface are non-null, and only then read w = sdlCtx->mSurface->w and h = sdlCtx->mSurface->h and return success (otherwise return ErrorCode); update the function to use BackendContext and its mHandle rather than casting ctx.mBackendCtx directly.
409-417:⚠️ Potential issue | 🔴 Critical
closeScreenshould respect ownership and free backend wrappers.On Lines 416-417, the code always destroys the window and only nulls
mWindow. This ignores ownership (mOwnerset false on Line 348 for externally provided handles) and leaks bothSDLContextandBackendContext.🔧 Proposed fix
ret_code Renderer::closeScreen(Context &ctx) { - SDLContext *sdlCtx = getBackendContext(ctx); - if (sdlCtx->mWindow == nullptr) { + if (ctx.mBackendCtx == nullptr || ctx.mBackendCtx->mHandle == nullptr) { return ErrorCode; } + SDLContext *sdlCtx = getBackendContext(ctx); TTF_Quit(); - SDL_DestroyWindow(sdlCtx->mWindow); - sdlCtx->mWindow = nullptr; + sdlCtx->destroy(); + delete ctx.mBackendCtx; + ctx.mBackendCtx = nullptr; + ctx.mCreated = false; return ResultOk; }🤖 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 409 - 417, In closeScreen, respect SDLContext::mOwner when cleaning up: obtain the SDLContext via getBackendContext(ctx), and only call SDL_DestroyWindow and null mWindow if sdlCtx->mOwner is true; if mOwner is false, do not destroy the external window. Regardless of ownership, ensure the backend wrapper objects are freed—release/delete the SDLContext and the BackendContext wrapper associated with ctx (and null out pointers) so neither SDLContext nor BackendContext leak. Reference symbols: closeScreen, getBackendContext, SDLContext::mWindow, SDLContext::mOwner, BackendContext.src/widgets.cpp (1)
224-224:⚠️ Potential issue | 🔴 CriticalStrengthen renderer readiness checks before widget creation.
On Line 224 (and the same pattern on Lines 246, 266, 292, 319, 339, 361, 391, 420, 456), checking only
ctx.mBackendCtxis not enough: the wrapper can exist while the underlying backend handle/renderer is still uninitialized. That can pass these guards and crash later in render paths.🔧 Proposed fix (centralized readiness guard)
+static bool isBackendReady(Context &ctx) { + if (ctx.mBackendCtx == nullptr || ctx.mBackendCtx->mHandle == nullptr) { + return false; + } + auto *sdlCtx = getBackendContext(ctx); + return sdlCtx != nullptr && sdlCtx->mRenderer != nullptr; +} + ret_code Widgets::label(Id id, Id parentId, const char *text, const Rect &rect, Alignment alignment) { auto &ctx = TinyUi::getContext(); - if (ctx.mBackendCtx == nullptr) { + if (!isBackendReady(ctx)) { return InvalidRenderHandle; }As per coding guidelines, "Validate inputs at function start".
Also applies to: 246-246, 266-266, 292-292, 319-319, 339-339, 361-361, 391-391, 420-420, 456-456
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.cpp` at line 224, The current checks like "if (ctx.mBackendCtx == nullptr)" are insufficient because the wrapper may exist while its underlying renderer/handle is uninitialized; add a centralized readiness guard function (e.g., bool ensureBackendReady(const WidgetContext& ctx) or BackendReadyOrLog(const WidgetContext& ctx)) that verifies ctx.mBackendCtx is non-null AND that the underlying backend handle/renderer (e.g., ctx.mBackendCtx->renderer, ctx.mBackendCtx->getHandle(), or ctx.mBackendCtx->isInitialized()) is valid, logs an error, and returns false; replace the simple null checks in the affected functions (the sites that currently reference ctx.mBackendCtx at lines noted) with a call to this helper and early-return on false so widget creation/render paths cannot proceed when the renderer is not ready.
🤖 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`:
- Around line 275-276: The initScreen path leaks BackendContext and SDLContext
on multiple early returns; ensure any early return cleans up: when you create
sdlCtx (SDLContext) but fail before allocating ctx.mBackendCtx, call
sdlCtx->destroy() and delete sdlCtx (or otherwise free it) before returning;
after allocating ctx.mBackendCtx and assigning mHandle, on every subsequent
failure (TTF_Init, loadDefaultFont, SDL_CreateWindow, driver query/getDriver,
SDL_CreateRenderer, surface acquisition) call sdlCtx->destroy(), delete
ctx.mBackendCtx (and set ctx.mBackendCtx = nullptr), and return; on full
successful init set sdlCtx->mOwner = true so sdlCtx->destroy() will later clean
up SDL window/renderer. Ensure all cleanup is symmetric and idempotent (safe to
call destroy/delete even if partially initialized).
---
Duplicate comments:
In `@src/backends/sdl2_renderer.cpp`:
- Around line 341-343: The code calls getBackendContext(ctx) which dereferences
ctx.mBackendCtx->mHandle before ctx.mBackendCtx is initialized; ensure the
backend context is created/validated at the start of this external init
overload: check that ctx.mBackendCtx is non-null (or allocate/assign a new
backend context whose mHandle is properly initialized) before calling
getBackendContext(ctx), then assign sdlCtx->mRenderer = renderer and
sdlCtx->mWindow = window; alternatively call the existing initializer function
that sets up ctx.mBackendCtx first (referencing getBackendContext, SDLContext,
ctx.mBackendCtx, and mHandle).
- Around line 517-524: Renderer::getSurfaceInfo currently casts ctx.mBackendCtx
directly to SDLContext* which is undefined; instead obtain the BackendContext
pointer from ctx.mBackendCtx, check it for null, then cast its mHandle member to
SDLContext* (e.g., SDLContext *sdlCtx =
static_cast<SDLContext*>(backendCtx->mHandle)), validate sdlCtx and
sdlCtx->mSurface are non-null, and only then read w = sdlCtx->mSurface->w and h
= sdlCtx->mSurface->h and return success (otherwise return ErrorCode); update
the function to use BackendContext and its mHandle rather than casting
ctx.mBackendCtx directly.
- Around line 409-417: In closeScreen, respect SDLContext::mOwner when cleaning
up: obtain the SDLContext via getBackendContext(ctx), and only call
SDL_DestroyWindow and null mWindow if sdlCtx->mOwner is true; if mOwner is
false, do not destroy the external window. Regardless of ownership, ensure the
backend wrapper objects are freed—release/delete the SDLContext and the
BackendContext wrapper associated with ctx (and null out pointers) so neither
SDLContext nor BackendContext leak. Reference symbols: closeScreen,
getBackendContext, SDLContext::mWindow, SDLContext::mOwner, BackendContext.
In `@src/widgets.cpp`:
- Line 224: The current checks like "if (ctx.mBackendCtx == nullptr)" are
insufficient because the wrapper may exist while its underlying renderer/handle
is uninitialized; add a centralized readiness guard function (e.g., bool
ensureBackendReady(const WidgetContext& ctx) or BackendReadyOrLog(const
WidgetContext& ctx)) that verifies ctx.mBackendCtx is non-null AND that the
underlying backend handle/renderer (e.g., ctx.mBackendCtx->renderer,
ctx.mBackendCtx->getHandle(), or ctx.mBackendCtx->isInitialized()) is valid,
logs an error, and returns false; replace the simple null checks in the affected
functions (the sites that currently reference ctx.mBackendCtx at lines noted)
with a call to this helper and early-return on false so widget creation/render
paths cannot proceed when the renderer is not ready.
🪄 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: 500aff60-1479-4061-988b-faa480b7dac7
📒 Files selected for processing (3)
src/backends/sdl2_renderer.cppsrc/tinyui.cppsrc/widgets.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tinyui.cpp
Comment on lines
+275
to
+276
| ctx.mBackendCtx = new BackendContext; | ||
| ctx.mBackendCtx->mHandle = (void*) sdlCtx; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
wc -l src/backends/sdl2_renderer.cppRepository: kimkulling/tiny_ui
Length of output: 96
🏁 Script executed:
# Read the context around lines 275-330 to see all early returns
sed -n '270,330p' src/backends/sdl2_renderer.cpp | cat -nRepository: kimkulling/tiny_ui
Length of output: 2490
🏁 Script executed:
# Look for the function signature and overall structure
grep -n "BackendContext\|SDLContext\|SDL2Renderer\|init\|Create" src/backends/sdl2_renderer.cpp | head -30Repository: kimkulling/tiny_ui
Length of output: 1877
🏁 Script executed:
# Get the complete initScreen function starting from line 264
sed -n '264,327p' src/backends/sdl2_renderer.cpp | cat -nRepository: kimkulling/tiny_ui
Length of output: 2591
🏁 Script executed:
# Search for SDLContext definition and destroy method
grep -n "struct SDLContext\|class SDLContext\|SDLContext::create\|SDLContext::destroy\|mBackendCtx" src/backends/sdl2_renderer.cppRepository: kimkulling/tiny_ui
Length of output: 312
🏁 Script executed:
# Find BackendContext definition
grep -rn "struct BackendContext\|class BackendContext" src/Repository: kimkulling/tiny_ui
Length of output: 102
🏁 Script executed:
# Check if there's a destructor or cleanup helper
grep -n "~SDLContext\|~BackendContext\|destroy\|cleanup" src/backends/sdl2_renderer.cppRepository: kimkulling/tiny_ui
Length of output: 44
🏁 Script executed:
# Check BackendContext definition in tinyui.h
sed -n '384,400p' src/tinyui.h | cat -nRepository: kimkulling/tiny_ui
Length of output: 1166
🏁 Script executed:
# Search for SDLContext definition
find src -type f \( -name "*.h" -o -name "*.hpp" -o -name "*.cpp" \) -exec grep -l "struct SDLContext\|class SDLContext" {} \;Repository: kimkulling/tiny_ui
Length of output: 103
🏁 Script executed:
# Search for SDLContext::create and destroy implementations
grep -rn "SDLContext::create\|SDLContext::destroy\|~SDLContext" src/Repository: kimkulling/tiny_ui
Length of output: 143
🏁 Script executed:
# Look for SDLContext definition
grep -B5 -A10 "struct SDLContext\|class SDLContext" src/backends/sdl2_renderer.cppRepository: kimkulling/tiny_ui
Length of output: 44
🏁 Script executed:
# Search for SDLContext in header files
cat src/backends/sdl2_renderer.hRepository: kimkulling/tiny_ui
Length of output: 4264
🏁 Script executed:
# Search more broadly for SDLContext
rg "SDLContext" src/ -A 5 -B 5 | head -100Repository: kimkulling/tiny_ui
Length of output: 5407
🏁 Script executed:
# Check the other initScreen overload (context version)
sed -n '328,365p' src/backends/sdl2_renderer.cpp | cat -nRepository: kimkulling/tiny_ui
Length of output: 1306
🏁 Script executed:
# Check releaseScreen to see cleanup patterns
grep -A 20 "releaseScreen" src/backends/sdl2_renderer.cpp | head -40Repository: kimkulling/tiny_ui
Length of output: 672
🏁 Script executed:
# Check if mOwner is set anywhere in the codebase
grep -n "mOwner" src/backends/sdl2_renderer.cppRepository: kimkulling/tiny_ui
Length of output: 93
🏁 Script executed:
# Verify mOwner usage in initScreen more carefully
sed -n '264,325p' src/backends/sdl2_renderer.cpp | grep -n "mOwner"Repository: kimkulling/tiny_ui
Length of output: 44
🏁 Script executed:
# Check how destroy() is actually used
grep -n "\.destroy()" src/backends/sdl2_renderer.cppRepository: kimkulling/tiny_ui
Length of output: 44
Fix memory leaks on early returns in initScreen initialization.
After line 275-276 allocate BackendContext and SDLContext, multiple early returns (TTF_Init, loadDefaultFont, SDL_CreateWindow, driver query, SDL_CreateRenderer, surface acquisition) exit without cleanup, leaking both allocations. Additionally, line 273 has an early return that leaks sdlCtx before BackendContext is even allocated.
Implement a cleanup path that deletes BackendContext and calls sdlCtx->destroy() on all failure paths. Note: also set sdlCtx->mOwner = true on successful initialization so that destroy() properly cleans up SDL resources (window/renderer).
Lines affected by this issue
- Line 273: Early return before BackendContext allocation
- Lines 278, 283, 297, 303, 310, 322: Early returns after BackendContext allocation
🤖 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 275 - 276, The initScreen path
leaks BackendContext and SDLContext on multiple early returns; ensure any early
return cleans up: when you create sdlCtx (SDLContext) but fail before allocating
ctx.mBackendCtx, call sdlCtx->destroy() and delete sdlCtx (or otherwise free it)
before returning; after allocating ctx.mBackendCtx and assigning mHandle, on
every subsequent failure (TTF_Init, loadDefaultFont, SDL_CreateWindow, driver
query/getDriver, SDL_CreateRenderer, surface acquisition) call
sdlCtx->destroy(), delete ctx.mBackendCtx (and set ctx.mBackendCtx = nullptr),
and return; on full successful init set sdlCtx->mOwner = true so
sdlCtx->destroy() will later clean up SDL window/renderer. Ensure all cleanup is
symmetric and idempotent (safe to call destroy/delete even if partially
initialized).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/backends/sdl2_renderer.cpp (3)
182-196:⚠️ Potential issue | 🟠 MajorMemory leak:
SDLContextandBackendContextare not freed inreleaseRenderer.After destroying the SDL renderer, neither the
SDLContextobject (allocated viaSDLContext::create()) nor theBackendContextwrapper (allocated vianew BackendContext) is deleted. This leaks memory on every renderer release.🛡️ Proposed fix to clean up allocations
SDLContext *sdlCtx = getBackendContext(ctx); if (sdlCtx->mRenderer != nullptr) { SDL_DestroyRenderer(sdlCtx->mRenderer); sdlCtx->mRenderer = nullptr; } + delete sdlCtx; + delete ctx.mBackendCtx; + ctx.mBackendCtx = nullptr; SDL_Quit();🤖 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 182 - 196, The releaseRenderer function leaks the SDLContext and its BackendContext wrapper after destroying the SDL renderer; after destroying sdlCtx->mRenderer you should free the SDLContext and BackendContext allocations: call the appropriate destructor/cleanup for SDLContext (delete or SDLContext::destroy() if provided) for the pointer returned by getBackendContext(ctx), then delete the BackendContext stored in ctx.mBackendCtx and set ctx.mBackendCtx to nullptr to avoid dangling pointers; ensure this cleanup happens only when ctx.mBackendCtx is non-null (the existing check around getBackendContext(ctx) applies).
330-331:⚠️ Potential issue | 🟡 MinorTypo: "Not initialzed." should be "Not initialized."
Lines 330 and 356 still have the typo
"Not initialzed."(missing 'i'), while line 266 was corrected to"Not initialized.". Please fix consistently.✏️ Proposed fix
- ctx.mLogger(LogSeverity::Error, "Not initialzed."); + ctx.mLogger(LogSeverity::Error, "Not initialized.");🤖 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 330 - 331, The logger message contains a typo: replace the string literal "Not initialzed." with "Not initialized." wherever it's used (e.g., the ctx.mLogger(LogSeverity::Error, "Not initialzed.") calls) so the error message is consistent with the corrected occurrence; search for all occurrences of the exact "Not initialzed." literal and update them to "Not initialized." to ensure uniform messaging.
354-365:⚠️ Potential issue | 🟠 MajorMissing null check and incomplete cleanup in
releaseScreen.
getBackendContext(ctx)is called without verifyingctx.mBackendCtxis not null- Only the SDL window is destroyed;
SDLContextandBackendContextare not freed🛡️ Proposed fix
ret_code Renderer::releaseScreen(Context &ctx) { if (!ctx.mCreated) { ctx.mLogger(LogSeverity::Error, "Not initialzed."); return ErrorCode; } + if (ctx.mBackendCtx == nullptr) { + return ErrorCode; + } SDLContext *sdlCtx = getBackendContext(ctx); SDL_DestroyWindow(sdlCtx->mWindow); sdlCtx->mWindow = nullptr; + delete sdlCtx; + delete ctx.mBackendCtx; + ctx.mBackendCtx = nullptr; return ResultOk; }🤖 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 354 - 365, In releaseScreen, first verify ctx.mBackendCtx is non-null before calling getBackendContext(ctx) and log an error/return if it's null; then obtain SDLContext* sdlCtx = getBackendContext(ctx), check sdlCtx->mWindow for null before calling SDL_DestroyWindow and set sdlCtx->mWindow = nullptr; after destroying window, free the SDLContext/BackendContext (delete or appropriate free for SDLContext) and set ctx.mBackendCtx = nullptr and ctx.mCreated = false to complete cleanup and avoid dangling pointers.
♻️ Duplicate comments (4)
src/backends/sdl2_renderer.cpp (4)
517-527:⚠️ Potential issue | 🔴 CriticalCritical bug: Incorrect cast — casts
mBackendCtxinstead ofmBackendCtx->mHandle.Line 518 casts
ctx.mBackendCtxdirectly toSDLContext*, butmBackendCtxis aBackendContext*. The correct cast should be onmBackendCtx->mHandle, consistent withgetBackendContext().This causes undefined behavior when accessing
sdlCtx->mSurface.🐛 Proposed fix
ret_code Renderer::getSurfaceInfo(Context &ctx, int32_t &w, int32_t &h) { - SDLContext *sdlCtx = (SDLContext *) ctx.mBackendCtx; + if (ctx.mBackendCtx == nullptr) { + return ErrorCode; + } + const auto *sdlCtx = static_cast<const SDLContext*>(ctx.mBackendCtx->mHandle); if (sdlCtx->mSurface == nullptr) { return ErrorCode; }🤖 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 517 - 527, In Renderer::getSurfaceInfo, the code incorrectly casts ctx.mBackendCtx to SDLContext*, causing undefined behavior; instead obtain the BackendContext pointer (ctx.mBackendCtx), cast its mHandle to SDLContext* (as done in getBackendContext()), check that both the BackendContext and the resulting SDLContext* are non-null before accessing mSurface, then read w/h from sdlCtx->mSurface and return ResultOk (or return ErrorCode if any nulls or mSurface is null).
341-348:⚠️ Potential issue | 🔴 CriticalNull pointer dereference:
getBackendContext(ctx)called beforemBackendCtxis allocated.In this
initScreenoverload,getBackendContext(ctx)is called at line 341, butctx.mBackendCtxhas never been allocated. The helpergetBackendContextdereferencesctx.mBackendCtx->mHandleunconditionally, causing undefined behavior.🐛 Proposed fix: Allocate BackendContext before use
TTF_Init(); + ctx.mBackendCtx = new BackendContext; + SDLContext *sdlCtx = SDLContext::create(); + ctx.mBackendCtx->mHandle = static_cast<void*>(sdlCtx); - SDLContext *sdlCtx = getBackendContext(ctx); sdlCtx->mRenderer = renderer; sdlCtx->mWindow = window;🤖 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 341 - 348, The code calls getBackendContext(ctx) before ctx.mBackendCtx is allocated, causing a null dereference; fix initScreen overload by allocating and initializing ctx.mBackendCtx (creating a BackendContext instance and setting its mHandle appropriately) before calling getBackendContext(ctx), then proceed to set sdlCtx = getBackendContext(ctx) and assign mRenderer/mWindow/mSurface/mOwner; ensure allocation uses the same BackendContext/SDLContext types and ownership semantics as other init paths.
408-420:⚠️ Potential issue | 🟠 MajorResource leak:
closeScreendoes not clean upSDLContextorBackendContext.The function destroys
mWindowbut does not:
- Destroy the SDL renderer (
mRenderer)- Free the
SDLContextheap object- Free the
BackendContextwrapper- Set
ctx.mBackendCtxtonullptr🛡️ Proposed fix: Complete cleanup
ret_code Renderer::closeScreen(Context &ctx) { + if (ctx.mBackendCtx == nullptr) { + return ErrorCode; + } SDLContext *sdlCtx = getBackendContext(ctx); if (sdlCtx->mWindow == nullptr) { return ErrorCode; } TTF_Quit(); - SDL_DestroyWindow(sdlCtx->mWindow); - sdlCtx->mWindow = nullptr; + if (sdlCtx->mRenderer != nullptr) { + SDL_DestroyRenderer(sdlCtx->mRenderer); + } + SDL_DestroyWindow(sdlCtx->mWindow); + delete sdlCtx; + delete ctx.mBackendCtx; + ctx.mBackendCtx = nullptr; return ResultOk; }🤖 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 408 - 420, Renderer::closeScreen currently only destroys mWindow and quits TTF but leaks mRenderer, the SDLContext heap object, and the BackendContext wrapper; update Renderer::closeScreen to also call SDL_DestroyRenderer on sdlCtx->mRenderer (if non-null) and set it to nullptr, free/delete the SDLContext instance (sdlCtx) and the enclosing BackendContext wrapper, and set ctx.mBackendCtx to nullptr after cleanup; ensure you check pointers for null before destroying and perform TTF_Quit and SDL cleanup in the correct order to avoid use-after-free.
269-323:⚠️ Potential issue | 🔴 CriticalMemory leaks on multiple early return paths in
initScreen.The function allocates
SDLContextat line 269 andBackendContextat lines 275-276, but multiple early returns leak these allocations:
- Line 272: If
sdlCtx->mWindow != nullptr, returns without freeingsdlCtx- Lines 278-285, 297, 303, 310, 322: After
BackendContextallocation, all early returns leak both objectsStatic analysis also flags: "Potential leak of memory pointed to by 'sdlCtx'" at line 271.
🐛 Proposed fix: Add cleanup on failure paths
ret_code Renderer::initScreen(Context &ctx, int32_t x, int32_t y, int32_t w, int32_t h) { if (!ctx.mCreated) { ctx.mLogger(LogSeverity::Error, "Not initialized."); return ErrorCode; } SDLContext *sdlCtx = SDLContext::create(); if (sdlCtx->mWindow != nullptr) { ctx.mLogger(LogSeverity::Error, "Already created."); + delete sdlCtx; return ErrorCode; } ctx.mBackendCtx = new BackendContext; ctx.mBackendCtx->mHandle = (void*) sdlCtx; + auto cleanup = [&ctx, sdlCtx]() { + delete sdlCtx; + delete ctx.mBackendCtx; + ctx.mBackendCtx = nullptr; + }; + if (TTF_Init() == -1) { ctx.mLogger(LogSeverity::Error, "TTF init failed."); + cleanup(); return ErrorCode; } if (loadDefaultFont(ctx) == nullptr) { ctx.mLogger(LogSeverity::Error, "Cannot load default font."); + cleanup(); return ErrorCode; } // ... apply cleanup() to all other early returns ...🤖 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 269 - 323, The initScreen function leaks SDLContext (sdlCtx) and BackendContext (ctx.mBackendCtx) on multiple early returns; modify initScreen so that after allocating sdlCtx = SDLContext::create() and ctx.mBackendCtx = new BackendContext you clean up both on every error path (e.g., when sdlCtx->mWindow != nullptr, when TTF_Init()/loadDefaultFont/SDL_CreateWindow/queryDriver/SDL_CreateRenderer/SDL_GetWindowSurface fail): free/delete the SDLContext instance (using the matching destructor/cleanup for SDLContext::create()) and delete ctx.mBackendCtx, then set ctx.mBackendCtx and ctx.mBackendCtx->mHandle (or ctx.mBackendCtx pointer) to nullptr before returning; alternatively convert these raw allocations to RAII (unique_ptr or a scoped wrapper) to ensure automatic cleanup if you prefer.
🧹 Nitpick comments (1)
src/backends/sdl2_renderer.cpp (1)
374-376: Consider adding null safety togetBackendContexthelper.Multiple rendering functions call
getBackendContext(ctx)without checking ifctx.mBackendCtxis null. Rather than adding checks in every caller, consider making the helper defensive or returningnullptrwhen the backend context is uninitialized.As per coding guidelines: "Validate inputs at function start".
♻️ Suggested improvement to helper in sdl2_renderer.h
inline SDLContext *getBackendContext(Context &ctx) { if (ctx.mBackendCtx == nullptr || ctx.mBackendCtx->mHandle == nullptr) { return nullptr; } return static_cast<SDLContext*>(ctx.mBackendCtx->mHandle); }Then callers can check for
nullptrreturn:SDLContext *sdlCtx = getBackendContext(ctx); if (sdlCtx == nullptr) { return ErrorCode; }🤖 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 374 - 376, The helper getBackendContext currently assumes ctx.mBackendCtx is valid and callers (e.g., in sdl2_renderer.cpp where SDLContext *sdlCtx = getBackendContext(ctx) is used) dereference it without checks; make getBackendContext defensive by validating ctx.mBackendCtx and its mHandle and return nullptr when uninitialized (return static_cast<SDLContext*>(... ) only when valid), then update callers to check for a nullptr SDLContext (e.g., in rendering functions) and return an appropriate error code or no-op when nullptr is returned to avoid dereferencing a null pointer.
🤖 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`:
- Around line 182-196: The releaseRenderer function leaks the SDLContext and its
BackendContext wrapper after destroying the SDL renderer; after destroying
sdlCtx->mRenderer you should free the SDLContext and BackendContext allocations:
call the appropriate destructor/cleanup for SDLContext (delete or
SDLContext::destroy() if provided) for the pointer returned by
getBackendContext(ctx), then delete the BackendContext stored in ctx.mBackendCtx
and set ctx.mBackendCtx to nullptr to avoid dangling pointers; ensure this
cleanup happens only when ctx.mBackendCtx is non-null (the existing check around
getBackendContext(ctx) applies).
- Around line 330-331: The logger message contains a typo: replace the string
literal "Not initialzed." with "Not initialized." wherever it's used (e.g., the
ctx.mLogger(LogSeverity::Error, "Not initialzed.") calls) so the error message
is consistent with the corrected occurrence; search for all occurrences of the
exact "Not initialzed." literal and update them to "Not initialized." to ensure
uniform messaging.
- Around line 354-365: In releaseScreen, first verify ctx.mBackendCtx is
non-null before calling getBackendContext(ctx) and log an error/return if it's
null; then obtain SDLContext* sdlCtx = getBackendContext(ctx), check
sdlCtx->mWindow for null before calling SDL_DestroyWindow and set
sdlCtx->mWindow = nullptr; after destroying window, free the
SDLContext/BackendContext (delete or appropriate free for SDLContext) and set
ctx.mBackendCtx = nullptr and ctx.mCreated = false to complete cleanup and avoid
dangling pointers.
---
Duplicate comments:
In `@src/backends/sdl2_renderer.cpp`:
- Around line 517-527: In Renderer::getSurfaceInfo, the code incorrectly casts
ctx.mBackendCtx to SDLContext*, causing undefined behavior; instead obtain the
BackendContext pointer (ctx.mBackendCtx), cast its mHandle to SDLContext* (as
done in getBackendContext()), check that both the BackendContext and the
resulting SDLContext* are non-null before accessing mSurface, then read w/h from
sdlCtx->mSurface and return ResultOk (or return ErrorCode if any nulls or
mSurface is null).
- Around line 341-348: The code calls getBackendContext(ctx) before
ctx.mBackendCtx is allocated, causing a null dereference; fix initScreen
overload by allocating and initializing ctx.mBackendCtx (creating a
BackendContext instance and setting its mHandle appropriately) before calling
getBackendContext(ctx), then proceed to set sdlCtx = getBackendContext(ctx) and
assign mRenderer/mWindow/mSurface/mOwner; ensure allocation uses the same
BackendContext/SDLContext types and ownership semantics as other init paths.
- Around line 408-420: Renderer::closeScreen currently only destroys mWindow and
quits TTF but leaks mRenderer, the SDLContext heap object, and the
BackendContext wrapper; update Renderer::closeScreen to also call
SDL_DestroyRenderer on sdlCtx->mRenderer (if non-null) and set it to nullptr,
free/delete the SDLContext instance (sdlCtx) and the enclosing BackendContext
wrapper, and set ctx.mBackendCtx to nullptr after cleanup; ensure you check
pointers for null before destroying and perform TTF_Quit and SDL cleanup in the
correct order to avoid use-after-free.
- Around line 269-323: The initScreen function leaks SDLContext (sdlCtx) and
BackendContext (ctx.mBackendCtx) on multiple early returns; modify initScreen so
that after allocating sdlCtx = SDLContext::create() and ctx.mBackendCtx = new
BackendContext you clean up both on every error path (e.g., when sdlCtx->mWindow
!= nullptr, when
TTF_Init()/loadDefaultFont/SDL_CreateWindow/queryDriver/SDL_CreateRenderer/SDL_GetWindowSurface
fail): free/delete the SDLContext instance (using the matching
destructor/cleanup for SDLContext::create()) and delete ctx.mBackendCtx, then
set ctx.mBackendCtx and ctx.mBackendCtx->mHandle (or ctx.mBackendCtx pointer) to
nullptr before returning; alternatively convert these raw allocations to RAII
(unique_ptr or a scoped wrapper) to ensure automatic cleanup if you prefer.
---
Nitpick comments:
In `@src/backends/sdl2_renderer.cpp`:
- Around line 374-376: The helper getBackendContext currently assumes
ctx.mBackendCtx is valid and callers (e.g., in sdl2_renderer.cpp where
SDLContext *sdlCtx = getBackendContext(ctx) is used) dereference it without
checks; make getBackendContext defensive by validating ctx.mBackendCtx and its
mHandle and return nullptr when uninitialized (return
static_cast<SDLContext*>(... ) only when valid), then update callers to check
for a nullptr SDLContext (e.g., in rendering functions) and return an
appropriate error code or no-op when nullptr is returned to avoid dereferencing
a null pointer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32f8f0b2-b74c-4cb6-9665-43c0baacb0f7
📒 Files selected for processing (1)
src/backends/sdl2_renderer.cpp
Updated error logging messages for better clarity.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backends/sdl2_renderer.cpp (1)
184-196:⚠️ Potential issue | 🟠 MajorIncomplete cleanup:
BackendContextandSDLContextare not freed.The function destroys
mRendererbut does not free theSDLContextobject or deletectx.mBackendCtx. This can lead to memory leaks ifreleaseRendereris called without a corresponding cleanup path.🛡️ Proposed fix: Complete the cleanup
if (ctx.mBackendCtx == nullptr) { SDL_Quit(); return ResultOk; } - SDLContext *sdlCtx = getBackendContext(ctx); - if (sdlCtx->mRenderer != nullptr) { + if (SDLContext *sdlCtx = getBackendContext(ctx); sdlCtx != nullptr) { + if (sdlCtx->mRenderer != nullptr) { - SDL_DestroyRenderer(sdlCtx->mRenderer); - sdlCtx->mRenderer = nullptr; + SDL_DestroyRenderer(sdlCtx->mRenderer); + sdlCtx->mRenderer = nullptr; + } + SDLContext::destroy(sdlCtx); } + delete ctx.mBackendCtx; + ctx.mBackendCtx = nullptr; SDL_Quit();🤖 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 184 - 196, The cleanup currently destroys sdlCtx->mRenderer but never frees the SDLContext or clears ctx.mBackendCtx; after destroying the renderer (the block handling sdlCtx->mRenderer in getBackendContext), delete the SDLContext object (delete sdlCtx) and set ctx.mBackendCtx = nullptr (or free it with the appropriate deallocator if not created with new) before calling SDL_Quit so both the SDLContext and the backend context pointer are released and nullified to avoid leaks.
♻️ Duplicate comments (4)
src/backends/sdl2_renderer.cpp (4)
522-532:⚠️ Potential issue | 🔴 CriticalCritical bug: Incorrect cast causes undefined behavior.
This issue was previously flagged. Line 523 casts
ctx.mBackendCtx(aBackendContext*) directly toSDLContext*, but this is incorrect. TheSDLContext*is stored inctx.mBackendCtx->mHandle, not inctx.mBackendCtxitself.This will cause undefined behavior when accessing
sdlCtx->mSurface.🐛 Proposed fix
ret_code Renderer::getSurfaceInfo(Context &ctx, int32_t &w, int32_t &h) { - SDLContext *sdlCtx = (SDLContext *) ctx.mBackendCtx; + if (ctx.mBackendCtx == nullptr) { + return ErrorCode; + } + const auto *sdlCtx = static_cast<const SDLContext*>(ctx.mBackendCtx->mHandle); + if (sdlCtx == nullptr) { + return ErrorCode; + } if (sdlCtx->mSurface == nullptr) { return ErrorCode; }🤖 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 522 - 532, Renderer::getSurfaceInfo incorrectly casts ctx.mBackendCtx to SDLContext*, causing undefined behavior; instead obtain the BackendContext pointer from ctx.mBackendCtx, then cast its mHandle to SDLContext* (e.g., SDLContext *sdlCtx = (SDLContext*)backendCtx->mHandle), check both backendCtx and sdlCtx for nullptr before accessing sdlCtx->mSurface, and return ErrorCode if any are null; keep the existing assignments to w/h from sdlCtx->mSurface->w/h and return ResultOk on success.
413-425:⚠️ Potential issue | 🟠 MajorResource leak: Incomplete cleanup in
closeScreen.This issue was previously flagged. The function:
- Does not check if
sdlCtxis null before dereferencing at line 415- Only destroys
mWindow, notmRenderer- Does not free
SDLContextorBackendContext- Does not set
ctx.mBackendCtx = nullptr🛡️ Proposed fix: Complete cleanup
ret_code Renderer::closeScreen(Context &ctx) { SDLContext *sdlCtx = getBackendContext(ctx); + if (sdlCtx == nullptr) { + return ErrorCode; + } if (sdlCtx->mWindow == nullptr) { return ErrorCode; } TTF_Quit(); - SDL_DestroyWindow(sdlCtx->mWindow); - sdlCtx->mWindow = nullptr; + SDLContext::destroy(sdlCtx); + delete ctx.mBackendCtx; + ctx.mBackendCtx = nullptr; return ResultOk; }🤖 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 413 - 425, Renderer::closeScreen currently dereferences getBackendContext(ctx) without null-check, destroys only mWindow, and leaves backend context memory/live pointers intact; update closeScreen to first verify SDLContext* sdlCtx = getBackendContext(ctx) is non-null and return ErrorCode if null, then if sdlCtx->mRenderer != nullptr call SDL_DestroyRenderer and set it to nullptr, then if sdlCtx->mWindow != nullptr call SDL_DestroyWindow and set it to nullptr, call TTF_Quit() (or appropriate teardown) after destroying renderer/window, free/delete the SDLContext/BackendContext instance and set ctx.mBackendCtx = nullptr to avoid dangling pointer, and ensure the function returns ResultOk on successful full cleanup and ErrorCode on early failures.
269-326:⚠️ Potential issue | 🔴 CriticalMemory leaks on multiple early return paths.
This issue was previously flagged. The code creates
SDLContextat line 269 andBackendContextat lines 275-276, but multiple early returns leak these allocations:
- Line 272: Returns
ErrorCodewithout freeingsdlCtx- Lines 278-286, 297, 303, 310, 322: Return after
BackendContextallocation without cleanupAdditionally,
sdlCtx->mOwneris never set totrueon the success path, which may affect proper cleanup indestroy().🐛 Proposed fix: Add cleanup on failure paths
ret_code Renderer::initScreen(Context &ctx, int32_t x, int32_t y, int32_t w, int32_t h) { if (!ctx.mCreated) { ctx.mLogger(LogSeverity::Error, "Not initialized."); return ErrorCode; } SDLContext *sdlCtx = SDLContext::create(); if (sdlCtx->mWindow != nullptr) { ctx.mLogger(LogSeverity::Error, "Already created."); + SDLContext::destroy(sdlCtx); return ErrorCode; } ctx.mBackendCtx = new BackendContext; ctx.mBackendCtx->mHandle = (void*) sdlCtx; if (TTF_Init() == -1) { ctx.mLogger(LogSeverity::Error, "TTF init failed."); - return ErrorCode; + goto cleanup_fail; } // ... similar changes for other early returns ... + sdlCtx->mOwner = true; return ResultOk; + +cleanup_fail: + SDLContext::destroy(sdlCtx); + delete ctx.mBackendCtx; + ctx.mBackendCtx = nullptr; + return ErrorCode; }🤖 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 269 - 326, The function creates SDLContext (SDLContext::create()) and allocates BackendContext but returns early on several failure paths without freeing those allocations and without setting sdlCtx->mOwner on success; update the early-return branches (after SDLContext creation and after ctx.mBackendCtx allocation) to free/cleanup any partially created resources (delete/free ctx.mBackendCtx, delete/free sdlCtx and call SDL_DestroyWindow/SDL_DestroyRenderer/SDL_FreeSurface as appropriate, and undo TTF_Init if needed) before returning ErrorCode, and set sdlCtx->mOwner = true on the successful path just before returning ResultOk so destroy() can clean up correctly; locate fixes around SDLContext::create(), the BackendContext assignment (ctx.mBackendCtx->mHandle), TTF_Init check, loadDefaultFont check, SDL_CreateWindow/SDL_CreateRenderer/SDL_GetWindowSurface failure checks, and the final return ResultOk.
341-357:⚠️ Potential issue | 🔴 Critical
BackendContextnever allocated in this overload — initialization will always fail.This issue was previously flagged. While a null check was added at lines 342-345, the root cause remains:
ctx.mBackendCtxis never allocated in this function, sogetBackendContext(ctx)will always returnnullptrand the function will always returnErrorCode.🐛 Proposed fix: Allocate BackendContext before use
TTF_Init(); + ctx.mBackendCtx = new BackendContext; + SDLContext *sdlCtx = SDLContext::create(); + ctx.mBackendCtx->mHandle = static_cast<void*>(sdlCtx); - SDLContext *sdlCtx = getBackendContext(ctx); - if (sdlCtx == nullptr) { - ctx.mLogger(LogSeverity::Error, "Invalid sdl context detected."); - return ErrorCode; - } sdlCtx->mRenderer = renderer; sdlCtx->mWindow = window;🤖 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 341 - 357, getBackendContext(ctx) is returning nullptr because ctx.mBackendCtx is never allocated in this overload; allocate and attach a BackendContext (SDLContext) to ctx before using it. Fix: if getBackendContext(ctx) returns nullptr, create a new SDLContext, assign it to ctx.mBackendCtx, then re-read sdlCtx and proceed to set mRenderer, mWindow, mSurface, mOwner and ctx.mCreated; ensure you set mOwner correctly for ownership semantics (e.g., true if we allocated it here), and use the same symbols shown (getBackendContext, ctx.mBackendCtx, SDLContext/sdlCtx, mRenderer, mWindow, mSurface, mOwner, ctx.mCreated).
🧹 Nitpick comments (3)
src/backends/sdl2_renderer.cpp (3)
427-432: Consider null check onsdlCtx.Same pattern as other render functions - defensive null check recommended.
🤖 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 427 - 432, In Renderer::endRender, add a defensive null check after obtaining SDLContext via getBackendContext(ctx) to avoid dereferencing a null pointer; if sdlCtx (or sdlCtx->mRenderer) is null, return the appropriate error result (e.g., ResultErr) instead of calling SDL_RenderPresent, otherwise call SDL_RenderPresent(sdlCtx->mRenderer) and return ResultOk. Ensure you reference getBackendContext, SDLContext, sdlCtx, mRenderer, SDL_RenderPresent, and ResultOk/ResultErr in the fix.
405-407: Same missing validation asdrawRect.Add
ctx.mCreatedandsdlCtxnull checks for consistency and safety.🤖 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 405 - 407, The snippet using getBackendContext(ctx) and SDL_CreateTextureFromSurface lacks the same safety checks as drawRect: ensure ctx.mCreated is true and that sdlCtx (result of getBackendContext) is non-null before creating the SDL_Texture or calling SDL_RenderCopy; if either check fails, bail out early (return or no-op) to avoid dereferencing a null pointer or using an uninitialized context. Specifically, guard the calls around getBackendContext, SDL_CreateTextureFromSurface, and SDL_RenderCopy, and handle a null SDL_Texture from SDL_CreateTextureFromSurface appropriately.
379-381: Consider adding null check onsdlCtx.While
ctx.mCreatedprovides some protection, a defensive null check onsdlCtxwould prevent crashes if backend initialization failed partially.🤖 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 379 - 381, The code calls getBackendContext(ctx) and immediately uses sdlCtx->mRenderer in SDL_SetRenderDrawColor and SDL_RenderClear without checking that sdlCtx is non-null; add a defensive null check after SDLContext *sdlCtx = getBackendContext(ctx) (e.g., if (!sdlCtx) { /* log or return */ }) and bail out (return or skip rendering) when sdlCtx is null to avoid dereferencing a null pointer, ensuring you still respect the existing ctx.mCreated guard; reference SDLContext, getBackendContext, SDL_SetRenderDrawColor, and SDL_RenderClear when making the change.
🤖 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 330: Fix the spelling mistake in the TinyUI error messages by replacing
"initialzed" with "initialized" in the logger calls: update the string passed to
ctx.mLogger(LogSeverity::Error, "...") at the occurrence that reads "TinyUI
context is not initialzed." and also the other occurrence that reads "Not
initialzed." so both messages read "initialized" (search for ctx.mLogger and the
exact strings to locate both spots).
- Around line 365-368: getBackendContext(ctx) may return nullptr, and the
current code immediately dereferences sdlCtx (SDLContext *sdlCtx =
getBackendContext(ctx); SDL_DestroyWindow(sdlCtx->mWindow);), which can crash;
add a null check for sdlCtx before accessing sdlCtx->mWindow and calling
SDL_DestroyWindow (e.g., if sdlCtx == nullptr then return or handle the error),
and ensure you only set sdlCtx->mWindow = nullptr after SDL_DestroyWindow is
called on a valid sdlCtx.
- Around line 226-227: getBackendContext(ctx) may return nullptr so add a null
check for SDLContext* sdlCtx before dereferencing; if sdlCtx is nullptr, handle
the error (e.g., return an error code or clean up and exit the function) and
avoid calling SDL_CreateTextureFromSurface using sdlCtx->mRenderer (and
similarly avoid other uses of sdlCtx later). Specifically, guard the SDLContext*
sdlCtx = getBackendContext(ctx) result, log an appropriate error message, and
early-return to prevent dereferencing mRenderer in the
SDL_CreateTextureFromSurface call and any subsequent accesses.
- Around line 386-394: drawRect lacks the usual initialization guards: add a
check at the top of Renderer::drawRect to verify ctx.mCreated and return the
same error ret_code used by other render functions when not created, then call
getBackendContext(ctx) and check the returned SDLContext* (sdlCtx) for null and
return the same backend-failure ret_code used elsewhere if null; only proceed to
set color and call SDL_RenderFillRect/SDL_RenderDrawRect after those checks so
drawRect mirrors the safety of the other renderer methods.
---
Outside diff comments:
In `@src/backends/sdl2_renderer.cpp`:
- Around line 184-196: The cleanup currently destroys sdlCtx->mRenderer but
never frees the SDLContext or clears ctx.mBackendCtx; after destroying the
renderer (the block handling sdlCtx->mRenderer in getBackendContext), delete the
SDLContext object (delete sdlCtx) and set ctx.mBackendCtx = nullptr (or free it
with the appropriate deallocator if not created with new) before calling
SDL_Quit so both the SDLContext and the backend context pointer are released and
nullified to avoid leaks.
---
Duplicate comments:
In `@src/backends/sdl2_renderer.cpp`:
- Around line 522-532: Renderer::getSurfaceInfo incorrectly casts
ctx.mBackendCtx to SDLContext*, causing undefined behavior; instead obtain the
BackendContext pointer from ctx.mBackendCtx, then cast its mHandle to
SDLContext* (e.g., SDLContext *sdlCtx = (SDLContext*)backendCtx->mHandle), check
both backendCtx and sdlCtx for nullptr before accessing sdlCtx->mSurface, and
return ErrorCode if any are null; keep the existing assignments to w/h from
sdlCtx->mSurface->w/h and return ResultOk on success.
- Around line 413-425: Renderer::closeScreen currently dereferences
getBackendContext(ctx) without null-check, destroys only mWindow, and leaves
backend context memory/live pointers intact; update closeScreen to first verify
SDLContext* sdlCtx = getBackendContext(ctx) is non-null and return ErrorCode if
null, then if sdlCtx->mRenderer != nullptr call SDL_DestroyRenderer and set it
to nullptr, then if sdlCtx->mWindow != nullptr call SDL_DestroyWindow and set it
to nullptr, call TTF_Quit() (or appropriate teardown) after destroying
renderer/window, free/delete the SDLContext/BackendContext instance and set
ctx.mBackendCtx = nullptr to avoid dangling pointer, and ensure the function
returns ResultOk on successful full cleanup and ErrorCode on early failures.
- Around line 269-326: The function creates SDLContext (SDLContext::create())
and allocates BackendContext but returns early on several failure paths without
freeing those allocations and without setting sdlCtx->mOwner on success; update
the early-return branches (after SDLContext creation and after ctx.mBackendCtx
allocation) to free/cleanup any partially created resources (delete/free
ctx.mBackendCtx, delete/free sdlCtx and call
SDL_DestroyWindow/SDL_DestroyRenderer/SDL_FreeSurface as appropriate, and undo
TTF_Init if needed) before returning ErrorCode, and set sdlCtx->mOwner = true on
the successful path just before returning ResultOk so destroy() can clean up
correctly; locate fixes around SDLContext::create(), the BackendContext
assignment (ctx.mBackendCtx->mHandle), TTF_Init check, loadDefaultFont check,
SDL_CreateWindow/SDL_CreateRenderer/SDL_GetWindowSurface failure checks, and the
final return ResultOk.
- Around line 341-357: getBackendContext(ctx) is returning nullptr because
ctx.mBackendCtx is never allocated in this overload; allocate and attach a
BackendContext (SDLContext) to ctx before using it. Fix: if
getBackendContext(ctx) returns nullptr, create a new SDLContext, assign it to
ctx.mBackendCtx, then re-read sdlCtx and proceed to set mRenderer, mWindow,
mSurface, mOwner and ctx.mCreated; ensure you set mOwner correctly for ownership
semantics (e.g., true if we allocated it here), and use the same symbols shown
(getBackendContext, ctx.mBackendCtx, SDLContext/sdlCtx, mRenderer, mWindow,
mSurface, mOwner, ctx.mCreated).
---
Nitpick comments:
In `@src/backends/sdl2_renderer.cpp`:
- Around line 427-432: In Renderer::endRender, add a defensive null check after
obtaining SDLContext via getBackendContext(ctx) to avoid dereferencing a null
pointer; if sdlCtx (or sdlCtx->mRenderer) is null, return the appropriate error
result (e.g., ResultErr) instead of calling SDL_RenderPresent, otherwise call
SDL_RenderPresent(sdlCtx->mRenderer) and return ResultOk. Ensure you reference
getBackendContext, SDLContext, sdlCtx, mRenderer, SDL_RenderPresent, and
ResultOk/ResultErr in the fix.
- Around line 405-407: The snippet using getBackendContext(ctx) and
SDL_CreateTextureFromSurface lacks the same safety checks as drawRect: ensure
ctx.mCreated is true and that sdlCtx (result of getBackendContext) is non-null
before creating the SDL_Texture or calling SDL_RenderCopy; if either check
fails, bail out early (return or no-op) to avoid dereferencing a null pointer or
using an uninitialized context. Specifically, guard the calls around
getBackendContext, SDL_CreateTextureFromSurface, and SDL_RenderCopy, and handle
a null SDL_Texture from SDL_CreateTextureFromSurface appropriately.
- Around line 379-381: The code calls getBackendContext(ctx) and immediately
uses sdlCtx->mRenderer in SDL_SetRenderDrawColor and SDL_RenderClear without
checking that sdlCtx is non-null; add a defensive null check after SDLContext
*sdlCtx = getBackendContext(ctx) (e.g., if (!sdlCtx) { /* log or return */ })
and bail out (return or skip rendering) when sdlCtx is null to avoid
dereferencing a null pointer, ensuring you still respect the existing
ctx.mCreated guard; reference SDLContext, getBackendContext,
SDL_SetRenderDrawColor, and SDL_RenderClear when making the change.
🪄 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: 9977edf8-3e3a-441d-920d-925a3ff25087
📒 Files selected for processing (1)
src/backends/sdl2_renderer.cpp
Comment on lines
+226
to
+227
| SDLContext *sdlCtx = getBackendContext(ctx); | ||
| SDL_Texture *messageTexture = SDL_CreateTextureFromSurface(sdlCtx->mRenderer, surfaceMessage); |
There was a problem hiding this comment.
Missing null check on sdlCtx before dereferencing.
If getBackendContext(ctx) returns nullptr (e.g., backend not initialized), line 227 and 257 will dereference a null pointer.
🛡️ Proposed fix
SDLContext *sdlCtx = getBackendContext(ctx);
+ if (sdlCtx == nullptr) {
+ return ErrorCode;
+ }
SDL_Texture *messageTexture = SDL_CreateTextureFromSurface(sdlCtx->mRenderer, surfaceMessage);🤖 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 226 - 227,
getBackendContext(ctx) may return nullptr so add a null check for SDLContext*
sdlCtx before dereferencing; if sdlCtx is nullptr, handle the error (e.g.,
return an error code or clean up and exit the function) and avoid calling
SDL_CreateTextureFromSurface using sdlCtx->mRenderer (and similarly avoid other
uses of sdlCtx later). Specifically, guard the SDLContext* sdlCtx =
getBackendContext(ctx) result, log an appropriate error message, and
early-return to prevent dereferencing mRenderer in the
SDL_CreateTextureFromSurface call and any subsequent accesses.
| ret_code Renderer::initScreen(Context &ctx, SDL_Window *window, SDL_Renderer *renderer) { | ||
| if (!ctx.mCreated) { | ||
| ctx.mLogger(LogSeverity::Error, "Not initialzed."); | ||
| ctx.mLogger(LogSeverity::Error, "TinyUI context is not initialzed."); |
There was a problem hiding this comment.
Typo: "initialzed" should be "initialized".
The error message contains a spelling error.
✏️ Proposed fix
- ctx.mLogger(LogSeverity::Error, "TinyUI context is not initialzed.");
+ ctx.mLogger(LogSeverity::Error, "TinyUI context is not initialized.");Note: Line 361 has the same typo: "Not initialzed." should be "Not initialized."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ctx.mLogger(LogSeverity::Error, "TinyUI context is not initialzed."); | |
| ctx.mLogger(LogSeverity::Error, "TinyUI context is not initialized."); |
🤖 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 330, Fix the spelling mistake in the
TinyUI error messages by replacing "initialzed" with "initialized" in the logger
calls: update the string passed to ctx.mLogger(LogSeverity::Error, "...") at the
occurrence that reads "TinyUI context is not initialzed." and also the other
occurrence that reads "Not initialzed." so both messages read "initialized"
(search for ctx.mLogger and the exact strings to locate both spots).
Comment on lines
+365
to
368
| SDLContext *sdlCtx = getBackendContext(ctx); | ||
| SDL_DestroyWindow(sdlCtx->mWindow); | ||
| sdlCtx->mWindow = nullptr; | ||
|
|
There was a problem hiding this comment.
Missing null check on sdlCtx before dereferencing.
If getBackendContext(ctx) returns nullptr, line 366 will dereference a null pointer.
🛡️ Proposed fix
SDLContext *sdlCtx = getBackendContext(ctx);
+ if (sdlCtx == nullptr) {
+ return ErrorCode;
+ }
SDL_DestroyWindow(sdlCtx->mWindow);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| SDLContext *sdlCtx = getBackendContext(ctx); | |
| SDL_DestroyWindow(sdlCtx->mWindow); | |
| sdlCtx->mWindow = nullptr; | |
| SDLContext *sdlCtx = getBackendContext(ctx); | |
| if (sdlCtx == nullptr) { | |
| return ErrorCode; | |
| } | |
| SDL_DestroyWindow(sdlCtx->mWindow); | |
| sdlCtx->mWindow = nullptr; |
🤖 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 365 - 368,
getBackendContext(ctx) may return nullptr, and the current code immediately
dereferences sdlCtx (SDLContext *sdlCtx = getBackendContext(ctx);
SDL_DestroyWindow(sdlCtx->mWindow);), which can crash; add a null check for
sdlCtx before accessing sdlCtx->mWindow and calling SDL_DestroyWindow (e.g., if
sdlCtx == nullptr then return or handle the error), and ensure you only set
sdlCtx->mWindow = nullptr after SDL_DestroyWindow is called on a valid sdlCtx.
Comment on lines
386
to
394
| ret_code Renderer::drawRect(Context &ctx, int32_t x, int32_t y, int32_t w, int32_t h, bool filled, Color4 fg) { | ||
| SDL_Rect r = {x, y, w, h}; | ||
| SDL_SetRenderDrawColor(ctx.mSDLContext->mRenderer, fg.r, fg.g, fg.b, fg.a); | ||
| SDLContext *sdlCtx = getBackendContext(ctx); | ||
| SDL_SetRenderDrawColor(sdlCtx->mRenderer, fg.r, fg.g, fg.b, fg.a); | ||
| if (filled) { | ||
| SDL_RenderFillRect(ctx.mSDLContext->mRenderer, &r); | ||
| SDL_RenderFillRect(sdlCtx->mRenderer, &r); | ||
| } else { | ||
| SDL_RenderDrawRect(ctx.mSDLContext->mRenderer, &r); | ||
| SDL_RenderDrawRect(sdlCtx->mRenderer, &r); | ||
| } |
There was a problem hiding this comment.
Missing initialization and null checks.
Unlike other render functions, drawRect has no ctx.mCreated check and no null check on sdlCtx. This could crash if called before initialization or if backend setup failed.
🛡️ Proposed fix
ret_code Renderer::drawRect(Context &ctx, int32_t x, int32_t y, int32_t w, int32_t h, bool filled, Color4 fg) {
+ if (!ctx.mCreated) {
+ return ErrorCode;
+ }
SDL_Rect r = {x, y, w, h};
SDLContext *sdlCtx = getBackendContext(ctx);
+ if (sdlCtx == nullptr) {
+ return ErrorCode;
+ }
SDL_SetRenderDrawColor(sdlCtx->mRenderer, fg.r, fg.g, fg.b, fg.a);🤖 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 386 - 394, drawRect lacks the
usual initialization guards: add a check at the top of Renderer::drawRect to
verify ctx.mCreated and return the same error ret_code used by other render
functions when not created, then call getBackendContext(ctx) and check the
returned SDLContext* (sdlCtx) for null and return the same backend-failure
ret_code used elsewhere if null; only proceed to set color and call
SDL_RenderFillRect/SDL_RenderDrawRect after those checks so drawRect mirrors the
safety of the other renderer methods.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Improvements
Bug Fixes