Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors FindEmplace to use a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tx_service/include/cc/template_cc_map.h (1)
10055-10269: Fix compile error in BulkEmplaceForTest: pass address-of emplace parameter.Line 9028 passes
emplace(aboolvalue) toFindEmplace(), which expectsbool *emplace(a pointer). Change the call to use&emplace.Proposed fix
for (auto key : keys) { bool emplace = false; - auto it = FindEmplace(*key, emplace, false, false); + auto it = FindEmplace(*key, &emplace, false, false); if (!emplace) { assert(false);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tx_service/include/cc/template_cc_map.h`:
- Around line 10055-10061: The code unconditionally dereferences the emplace
pointer in FindEmplace, which can crash if nullptr; update FindEmplace to either
assert(emplace) at entry or check if (emplace) before writing to *emplace (and
do the same for every other write to *emplace in this file), e.g., set local
bool temp=false and assign to *emplace only when emplace!=nullptr, or use an
explicit EXPECT or assert at start of the function to enforce the non-null
contract; apply the same guard/assert to the other spot that writes to *emplace
(the other FindEmplace/related emplace-handling callsite).
| Iterator FindEmplace(const KeyT &key, | ||
| bool &emplace, | ||
| bool *emplace, | ||
| bool force_emplace = false, | ||
| bool read_only_req = false) | ||
| { | ||
| emplace = false; | ||
| *emplace = false; | ||
| bool fail_if_not_found = false; |
There was a problem hiding this comment.
Guard against null emplace pointer before dereference.
Line 10060 and Line 10268 unconditionally dereference emplace. With the new pointer-based contract, a nullptr call would crash. Consider guarding the writes (or asserting non-null) to make the API safer.
🛠️ Suggested fix
- *emplace = false;
+ if (emplace)
+ {
+ *emplace = false;
+ }
...
- *emplace = true;
+ if (emplace)
+ {
+ *emplace = true;
+ }Also applies to: 10267-10269
🤖 Prompt for AI Agents
In `@tx_service/include/cc/template_cc_map.h` around lines 10055 - 10061, The code
unconditionally dereferences the emplace pointer in FindEmplace, which can crash
if nullptr; update FindEmplace to either assert(emplace) at entry or check if
(emplace) before writing to *emplace (and do the same for every other write to
*emplace in this file), e.g., set local bool temp=false and assign to *emplace
only when emplace!=nullptr, or use an explicit EXPECT or assert at start of the
function to enforce the non-null contract; apply the same guard/assert to the
other spot that writes to *emplace (the other FindEmplace/related
emplace-handling callsite).
2 participants
Here are some reminders before you submit the pull request
fixes eloqdb/tx_service#issue_id./mtr --suite=mono_main,mono_multi,mono_basicSummary by CodeRabbit
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Improves correctness and resource management across CC and data sync paths.
FindEmplaceto acceptbool*(wasbool&); updates all call sites and internals; adds safeguard to not blocksequencetable updates when shard is fullAbortBlockCmdRequest()incc_entry.hlocal_cc_shards.cpp(whendata_sync_vecis empty or prior flush failed)SliceCoordinatorinitialization (removes union/destructor; always initializesmin_paused_key_andmin_paused_slice_index_)DLOG(INFO)in archive-read KEY_NOT_FOUND pathWritten by Cursor Bugbot for commit 790350f. This will update automatically on new commits. Configure here.