src: improve TSList iteration and erase operations - #517
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. Walkthrough
ChangesTSList size reporting
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR adds localized TSList iteration and erase behavior with accompanying tests, and no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ns-control-tower
left a comment
There was a problem hiding this comment.
Walkthrough
This PR extends the TSList template (both the object and pointer specializations) with a new for_each overload that passes the current list size to the callback, and changes erase to return the post-erase list size. The new size-aware for_each captures list_.size() once before iterating so every callback invocation sees a consistent snapshot. Tests cover both specializations for the new for_each and erase return-value behavior.
Changes
| File(s) | Summary |
|---|---|
src/nsolid/thread_safe.h |
Added size-aware for_each overload and changed erase return type from void to size_t in both TSList<DataType> and TSList<DataType*> specializations |
test/cctest/test_nsolid_thread_safe.cc |
Added four tests: ObjectForEachWithSize, ObjectEraseReturnsSize, PointerForEachWithSize, PointerEraseReturnsSize |
Assessment
erasereturn-type change is backward-compatible. The sole production caller,TracerImpl::removeHookinnsolid_trace.cc:254(trace_hook_list_.erase(it)), discards the return value, which is valid in C++ — no[[nodiscard]]/warn_unused_resultattribute is set onerase. No other production code callsTSList::erase(the remaining.erase()calls in the codebase are onLRUMap/std::maptypes, notTSList).for_eachoverload is additive and unambiguous. The new overload differs in arity from the existing one ((const DataType&, size_t)vs(const DataType&)), so overload resolution is clean. The test lambdas match each signature exactly.- Size snapshot is correct.
current_sizeis captured before the loop, giving all callbacks a consistent view. The mutex is non-recursive (init(true)→init(), notinit_recursive()), so a callback that re-enters aTSListmethod would deadlock rather than corrupt state — consistent with the existingfor_eachbehavior. - Tests are thorough and correct. Pointer tests properly manage memory (delete before erase, or save raw pointers before erase then delete). Edge cases (empty after all erases, size after each erase) are covered.
No blocking findings. No C++ compiler was available in the sandbox, but the change is a small, well-contained header template extension with comprehensive tests.
Verdict: APPROVE — clean, low-risk additive API extension with complete test coverage.
Add a size-aware for_each overload to TSList so callbacks can observe the list size while iterating. Return the updated size from erase and extend coverage for object and pointer specializations. Signed-off-by: Santiago Gimeno <santiago.gimeno@gmail.com> PR-URL: #517 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
f472092 to
92fa8c8
Compare
Add a size-aware for_each overload to TSList so callbacks can observe the list size while iterating. Return the updated size from erase and extend coverage for object and pointer specializations.
Summary by CodeRabbit
New Features
Tests