Use callbacks on heap and introduce reference counting#20
Conversation
WalkthroughIntroduces simple reference counting on CallbackI (mNumRefs, incRef, decRef) and updates widget code to inc/dec references. Sample switches from stack-allocated callbacks to heap-allocated callback instances to align with the new lifetime semantics. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 unit tests (beta)
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: 4
🧹 Nitpick comments (1)
samples/main.cpp (1)
101-102: Consider usingautofor cleaner type declarations.The type is already evident from the
newexpression. Usingautoreduces redundancy.♻️ Suggested change
- CallbackI *dynamicQuitCallback = new CallbackI(quit, (void*) &ctx); - CallbackI *dynamicUpdateProgressBarCallback = new CallbackI(updateProgressbar, nullptr, Events::UpdateEvent); + auto *dynamicQuitCallback = new CallbackI(quit, (void*) &ctx); + auto *dynamicUpdateProgressBarCallback = new CallbackI(updateProgressbar, nullptr, Events::UpdateEvent);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/main.cpp` around lines 101 - 102, Replace the explicit type declarations for the two allocations with auto so the compiler deduces the pointer type; e.g., change the declarations creating new CallbackI to use auto (preferably auto* for clarity) for dynamicQuitCallback and dynamicUpdateProgressBarCallback while keeping the constructor arguments (quit, (void*)&ctx) and (updateProgressbar, nullptr, Events::UpdateEvent) unchanged so the variables still point to the newly allocated CallbackI instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@samples/main.cpp`:
- Around line 100-105: The progressBar path leaks because Widgets::progressBar
assigns dynamicUpdateProgressBarCallback and pushes it into
ctx.mUpdateCallbackList without calling CallbackI::incRef(), leaving mNumRefs at
0 so decRef() won't delete it; update Widgets::progressBar (the assignment and
list-push logic that handles the callback) to call callback->incRef() when the
widget takes ownership (same place Widgets::button calls incRef()), or ensure
the code that adds to ctx.mUpdateCallbackList increments the refcount so
decRef() can later free the CallbackI instance.
In `@src/tinyui.h`:
- Around line 317-318: The member mNumRefs is initialized to 0 which breaks
intrusive reference-count semantics; change its initialization to 1 so that a
newly constructed callback object owns one reference by default (update the
declaration of uint32_t mNumRefs to initialize to 1); locate the member in the
callback class in src/tinyui.h and adjust the initializer for mNumRefs
accordingly so decRef() can be called without a prior explicit incRef().
- Around line 351-358: decRef() currently silently ignores calls when mNumRefs
is 0 and uses delete this, which is unsafe; change decRef to return a bool and
enforce/validate the counter: ensure mNumRefs is initialized correctly (or
assert mNumRefs>0 on entry), decrement mNumRefs and if it reaches 0 perform
deletion and return true, otherwise return false; update all callers of decRef
to handle the boolean result so they know the object may have been destroyed
(or, as an alternative, replace manual ref counting with
std::shared_ptr/std::enable_shared_from_this to eliminate delete this and manual
mNumRefs management).
In `@src/widgets.h`:
- Around line 97-100: The assigned callback stored in mCallback needs its
reference count incremented to match the existing decRef() in the destructor;
after you assign the new callback to mCallback (the same assignment around where
mCallback is set at that widget setter), immediately call mCallback->incRef() so
the object's mNumRefs is incremented and decRef() will later delete it properly;
ensure this mirrors the incRef()/decRef() pairing used in other places (e.g.,
the patterns around the earlier assignments that call incRef()).
---
Nitpick comments:
In `@samples/main.cpp`:
- Around line 101-102: Replace the explicit type declarations for the two
allocations with auto so the compiler deduces the pointer type; e.g., change the
declarations creating new CallbackI to use auto (preferably auto* for clarity)
for dynamicQuitCallback and dynamicUpdateProgressBarCallback while keeping the
constructor arguments (quit, (void*)&ctx) and (updateProgressbar, nullptr,
Events::UpdateEvent) unchanged so the variables still point to the newly
allocated CallbackI instances.
🪄 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: dcea9233-a925-4f5b-b68c-0bf79d24ce4a
📒 Files selected for processing (3)
samples/main.cppsrc/tinyui.hsrc/widgets.h
| // Allocate callbacks dynamically to ensure they persist during event handling | ||
| CallbackI *dynamicQuitCallback = new CallbackI(quit, (void*) &ctx); | ||
| CallbackI *dynamicUpdateProgressBarCallback = new CallbackI(updateProgressbar, nullptr, Events::UpdateEvent); | ||
|
|
||
| Widgets::button(7, RootPanelId, "Quit", Rect(100, 250, 100, 40), dynamicQuitCallback); | ||
| Widgets::progressBar(8, RootPanelId, Rect(100, 300, 100, 40), 50, dynamicUpdateProgressBarCallback); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how callbacks are assigned in widget creation functions
# Search for button implementation
echo "=== Searching for Widgets::button implementation ==="
ast-grep --pattern $'ret_code Widgets::button($$$) {
$$$
}'
# Search for progressBar implementation
echo "=== Searching for Widgets::progressBar implementation ==="
ast-grep --pattern $'ret_code Widgets::progressBar($$$) {
$$$
}'Repository: kimkulling/tiny_ui
Length of output: 172
🏁 Script executed:
#!/bin/bash
# Find the relevant files
echo "=== Finding main.cpp ==="
fd -t f "main.cpp"
echo "=== Finding tinyui.h ==="
fd -t f "tinyui.h"
echo "=== Finding CallbackI class definition ==="
rg "class CallbackI|struct CallbackI" -A 20Repository: kimkulling/tiny_ui
Length of output: 1971
🏁 Script executed:
#!/bin/bash
# Get the full CallbackI struct definition
echo "=== Full CallbackI definition from tinyui.h ==="
rg "struct CallbackI" -A 80 src/tinyui.h | head -120
echo ""
echo "=== Search for incRef and decRef methods ==="
rg "incRef|decRef" src/tinyui.h -A 3
echo ""
echo "=== Search for Widgets namespace and button/progressBar functions ==="
fd -t f "*.cpp" "*.h" -x rg "namespace Widgets|ret_code.*button|ret_code.*progressBar" {} + | head -30Repository: kimkulling/tiny_ui
Length of output: 3404
🏁 Script executed:
#!/bin/bash
# Search for Widgets functions
echo "=== Search for button function ==="
rg "button\(" src/ -B 2 -A 15 | head -50
echo ""
echo "=== Search for progressBar function ==="
rg "progressBar\(" src/ -B 2 -A 15 | head -50
echo ""
echo "=== Search for any calls to incRef ==="
rg "incRef\(\)" src/ samples/ -B 2 -A 2Repository: kimkulling/tiny_ui
Length of output: 4818
🏁 Script executed:
#!/bin/bash
# Get the full progressBar implementation
echo "=== Full progressBar implementation ==="
rg "ret_code Widgets::progressBar" -A 30 src/widgets.cpp
echo ""
echo "=== Check samples/main.cpp lines 95-110 ==="
sed -n '95,110p' samples/main.cppRepository: kimkulling/tiny_ui
Length of output: 1941
Only the progressBar callback will leak; the button callback is properly managed.
The review comment incorrectly states that incRef() is never called in Widgets::button(). The button() function does call callback->incRef() after assignment, so dynamicQuitCallback will be properly reference-counted and freed when the widget is destroyed.
However, Widgets::progressBar() does not call incRef(). It only assigns the callback and adds it to ctx.mUpdateCallbackList. This means dynamicUpdateProgressBarCallback starts with mNumRefs = 0, and when the widget is destroyed, decRef() will not delete it, causing a memory leak.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 101-101: Replace the use of "new" with an operation that automatically manages the memory.
[warning] 102-102: Replace the redundant type with "auto".
[failure] 102-102: Replace the use of "new" with an operation that automatically manages the memory.
[warning] 101-101: Replace the redundant type with "auto".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@samples/main.cpp` around lines 100 - 105, The progressBar path leaks because
Widgets::progressBar assigns dynamicUpdateProgressBarCallback and pushes it into
ctx.mUpdateCallbackList without calling CallbackI::incRef(), leaving mNumRefs at
0 so decRef() won't delete it; update Widgets::progressBar (the assignment and
list-push logic that handles the callback) to call callback->incRef() when the
widget takes ownership (same place Widgets::button calls incRef()), or ensure
the code that adds to ctx.mUpdateCallbackList increments the refcount so
decRef() can later free the CallbackI instance.
| /// The reference count for the callback interface, used for memory management. | ||
| uint32_t mNumRefs{ 0 }; |
There was a problem hiding this comment.
Initialize mNumRefs to 1 for correct reference counting semantics.
With mNumRefs initialized to 0, a newly created callback requires an explicit incRef() before the first decRef() will function correctly. The standard intrusive reference counting pattern initializes the count to 1 at construction time, representing the creator's ownership.
🐛 Proposed fix
/// The reference count for the callback interface, used for memory management.
- uint32_t mNumRefs{ 0 };
+ uint32_t mNumRefs{ 1 };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tinyui.h` around lines 317 - 318, The member mNumRefs is initialized to 0
which breaks intrusive reference-count semantics; change its initialization to 1
so that a newly constructed callback object owns one reference by default
(update the declaration of uint32_t mNumRefs to initialize to 1); locate the
member in the callback class in src/tinyui.h and adjust the initializer for
mNumRefs accordingly so decRef() can be called without a prior explicit
incRef().
| void decRef() { | ||
| if (mNumRefs > 0) { | ||
| --mNumRefs; | ||
| if (mNumRefs == 0) { | ||
| delete this; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
decRef() logic flaw and fragile delete this pattern.
Two concerns:
-
Logic issue: If
mNumRefsis0(initial value),decRef()silently does nothing. This means callbacks that were neverincRef()'d will leak. Either fix the initial value (see above) or adjust the guard. -
delete thisis error-prone: The caller has no indication that the object was destroyed, which can lead to use-after-free. Consider returning a boolean indicating whether the object was deleted, or using a weak reference pattern.
🛡️ Proposed safer implementation
/// `@brief` Decrement the reference count.
- void decRef() {
- if (mNumRefs > 0) {
- --mNumRefs;
- if (mNumRefs == 0) {
- delete this;
- }
- }
+ /// `@return` true if the object was deleted, false otherwise.
+ bool decRef() {
+ assert(mNumRefs > 0 && "decRef called on object with zero refcount");
+ --mNumRefs;
+ if (mNumRefs == 0) {
+ delete this;
+ return true;
+ }
+ return false;
}Alternatively, for modern C++17, consider using std::shared_ptr with a custom destructor or std::enable_shared_from_this to avoid manual reference counting entirely.
📝 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.
| void decRef() { | |
| if (mNumRefs > 0) { | |
| --mNumRefs; | |
| if (mNumRefs == 0) { | |
| delete this; | |
| } | |
| } | |
| } | |
| /// `@brief` Decrement the reference count. | |
| /// `@return` true if the object was deleted, false otherwise. | |
| bool decRef() { | |
| assert(mNumRefs > 0 && "decRef called on object with zero refcount"); | |
| --mNumRefs; | |
| if (mNumRefs == 0) { | |
| delete this; | |
| return true; | |
| } | |
| return false; | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 355-355: Rewrite the code so that you no longer need this "delete".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tinyui.h` around lines 351 - 358, decRef() currently silently ignores
calls when mNumRefs is 0 and uses delete this, which is unsafe; change decRef to
return a bool and enforce/validate the counter: ensure mNumRefs is initialized
correctly (or assert mNumRefs>0 on entry), decrement mNumRefs and if it reaches
0 perform deletion and return true, otherwise return false; update all callers
of decRef to handle the boolean result so they know the object may have been
destroyed (or, as an alternative, replace manual ref counting with
std::shared_ptr/std::enable_shared_from_this to eliminate delete this and manual
mNumRefs management).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/widgets.cpp (1)
466-473:⚠️ Potential issue | 🟠 MajorInconsistency:
progressBardoesn't callincRef()on callback.Unlike
button,imageButton, andtreeView, this function doesn't increment the callback's reference count. Since the widget destructor unconditionally callsdecRef(), this creates a reference count imbalance that could cause premature deletion or undefined behavior with shared callbacks.🔧 Proposed fix: add incRef to match other widget functions
child->mCallback = callback; + if (callback != nullptr) { + callback->incRef(); + } EventPayload payload;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.cpp` around lines 466 - 473, The progressBar widget sets child->mCallback = callback but fails to increment the callback's refcount, causing imbalance because the widget destructor calls decRef(); fix by calling callback->incRef() after assigning and before setting callback->mInstance and pushing it into ctx.mUpdateCallbackList so the lifecycle matches other widgets like button/imageButton/treeView (i.e., mirror their sequence: assign mCallback, incRef(), set mInstance, push to mUpdateCallbackList).
🤖 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/widgets.cpp`:
- Around line 299-303: Before assigning child->mCallback = callback, if
child->mCallback is non-null call child->mCallback->decRef() to release the old
reference, then assign the new callback and call callback->incRef() only if
callback is non-null; i.e., ensure the existing callback is decRef'ed prior to
reassignment (referencing child->mCallback, callback, decRef(), and incRef()) to
avoid the same reference-count leak as in button().
- Around line 273-276: When assigning a new callback to an existing widget you
must release the old reference first to avoid leaking refs: in the code path
where you set child->mCallback (e.g., in createWidget/where mCallback is
reassigned), if child->mCallback is non-null call child->mCallback->decRef()
before overwriting it, then assign child->mCallback = callback and if (callback
!= nullptr) call callback->incRef(); ensure you update every place that
reassigns mCallback so incRef/decRef are balanced.
---
Outside diff comments:
In `@src/widgets.cpp`:
- Around line 466-473: The progressBar widget sets child->mCallback = callback
but fails to increment the callback's refcount, causing imbalance because the
widget destructor calls decRef(); fix by calling callback->incRef() after
assigning and before setting callback->mInstance and pushing it into
ctx.mUpdateCallbackList so the lifecycle matches other widgets like
button/imageButton/treeView (i.e., mirror their sequence: assign mCallback,
incRef(), set mInstance, push to mUpdateCallbackList).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| child->mCallback = callback; | ||
| if (callback != nullptr) { | ||
| callback->incRef(); | ||
| } |
There was a problem hiding this comment.
Reference count leak when reassigning callback on existing widget.
createWidget can return an existing widget (lines 119-121). If that widget already has a callback, assigning the new callback overwrites mCallback without calling decRef() on the old one—leaking its reference count.
🔧 Proposed fix: decrement old callback before assignment
+ if (child->mCallback != nullptr) {
+ child->mCallback->decRef();
+ }
child->mCallback = callback;
if (callback != nullptr) {
callback->incRef();
}📝 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.
| child->mCallback = callback; | |
| if (callback != nullptr) { | |
| callback->incRef(); | |
| } | |
| if (child->mCallback != nullptr) { | |
| child->mCallback->decRef(); | |
| } | |
| child->mCallback = callback; | |
| if (callback != nullptr) { | |
| callback->incRef(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/widgets.cpp` around lines 273 - 276, When assigning a new callback to an
existing widget you must release the old reference first to avoid leaking refs:
in the code path where you set child->mCallback (e.g., in createWidget/where
mCallback is reassigned), if child->mCallback is non-null call
child->mCallback->decRef() before overwriting it, then assign child->mCallback =
callback and if (callback != nullptr) call callback->incRef(); ensure you update
every place that reassigns mCallback so incRef/decRef are balanced.
| child->mCallback = callback; | ||
| if (callback != nullptr) { | ||
| callback->incRef(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same reference count leak issue as button().
Apply the same fix here—call decRef() on the existing callback before reassignment.
🔧 Proposed fix
+ if (child->mCallback != nullptr) {
+ child->mCallback->decRef();
+ }
child->mCallback = callback;
if (callback != nullptr) {
callback->incRef();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/widgets.cpp` around lines 299 - 303, Before assigning child->mCallback =
callback, if child->mCallback is non-null call child->mCallback->decRef() to
release the old reference, then assign the new callback and call
callback->incRef() only if callback is non-null; i.e., ensure the existing
callback is decRef'ed prior to reassignment (referencing child->mCallback,
callback, decRef(), and incRef()) to avoid the same reference-count leak as in
button().
1 participant
Summary by CodeRabbit
Refactor
Bug Fixes