fix: remediate batch 3 clang-tidy violations#228
Conversation
Replace char* relational comparisons with std::uintptr_t integer comparisons to avoid undefined behavior when checking whether a pointer belongs to the pool buffer (comparing pointers to unrelated objects is UB per the standard). Ref: #227 (comment) Made-with: Cursor
Config:
- Add LocalConstantCase/LocalConstantPointerCase (lower_case) to
stop false-flagging local const variables like lock guards
- Add ProtectedMemberSuffix ('_') matching the private convention
- Add StaticConstantCase (UPPER_CASE) explicitly
Readability:
- Make implicit bool conversions explicit (14 sites across e2e,
platform, transport, and tp modules)
- Use = default for trivial copy constructors (Message, Endpoint)
- Convert iterator loop to range-based for (tp_reassembler)
Const-correctness:
- Add const to ~25 local variables that are never modified
(endpoint, tp_manager, tp_reassembler, tp_segmenter,
e2e_profile_registry, event_publisher)
Made-with: Cursor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR applies consistent code-style improvements across the codebase: expanding Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/tp/tp_reassembler.cpp`:
- Around line 52-53: The computation of tp_header uses left shifts on uint8_t
values which are promoted to int and can overflow in C++17; update the
expression that defines tp_header so each byte operand
(payload[16]..payload[19]) is cast to uint32_t (e.g.
static_cast<uint32_t>(payload[i])) before shifting and combine with | to form
the uint32_t tp_header in the tp_reassembler.cpp file.
In `@src/tp/tp_segmenter.cpp`:
- Around line 105-106: The subtraction of a fixed overhead from
config_.max_segment_size (used when computing first_payload_size and the similar
calculation at lines ~145-146) can underflow because size_t is unsigned; change
the logic to compute an effective_capacity by checking if
config_.max_segment_size > overhead (16 + 4) and using config_.max_segment_size
- overhead, otherwise 0, then set first_payload_size =
std::min(effective_capacity, static_cast<size_t>(total_length)); apply the same
guarded pattern to the other payload-size calculation so no unsigned underflow
can occur.
- Line 87: The sequence counter next_sequence_number_ is being advanced twice:
once by using post-increment when assigning uint8_t const sequence_number =
next_sequence_number_++; and again at the function end, which skips sequence
values; fix by not incrementing twice — either stop incrementing at the end
(remove the extra increment) or stop incrementing at assignment (change the
assignment to use the current value: use next_sequence_number_ without ++ and
let the single increment at the function end remain); update references to
sequence_number and next_sequence_number_ accordingly so only one increment
occurs per message.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6700c7a6-45b8-4a6a-8bbb-38e5f4a02209
📒 Files selected for processing (12)
.clang-tidysrc/e2e/e2e_crc.cppsrc/e2e/e2e_profile_registry.cppsrc/e2e/e2e_protection.cppsrc/events/event_publisher.cppsrc/platform/freertos/memory.cppsrc/platform/threadx/memory.cppsrc/someip/message.cppsrc/tp/tp_manager.cppsrc/tp/tp_reassembler.cppsrc/tp/tp_segmenter.cppsrc/transport/endpoint.cpp
- Cast uint8_t to uint32_t before left-shifting in TP header parsing to avoid signed integer overflow from int promotion - Fix double-increment of sequence number in create_multi_segments (post-increment at assignment + modular increment at end skipped one sequence value per call) - Guard unsigned subtraction in segment payload-size calculations to prevent underflow when max_segment_size < overhead Made-with: Cursor
Summary
Third batch of clang-tidy remediations targeting naming config, explicit bool conversions, const-correctness, and code modernization across 12 files.
Also includes the
std::uintptr_tfix from PR #227 review.Changes by category
Config (eliminates ~160 false-positive naming warnings)
LocalConstantCase: lower_case— stops flagging local const variables like lock guardsLocalConstantPointerCase: lower_case— same for local const pointersProtectedMemberSuffix: '_'— matches the existing private member conventionStaticConstantCase: UPPER_CASE— explicitly set for file-scope static constsFreeRTOS pool fix (from PR #227 review)
std::uintptr_tfor pool ownership bounds checks to avoid UB from comparing unrelated pointersReadability (17 fixes)
readability-implicit-bool-conversion: explicit!= nullptr/!= 0comparisons (e2e_crc, e2e_protection, freertos/memory, threadx/memory, tp_reassembler, endpoint)hicpp-use-equals-default: trivial copy constructors forMessageandEndpointmodernize-loop-convert: range-based for in tp_reassemblerConst-correctness (~25 fixes)
constto local variables that are never modified (endpoint, tp_manager, tp_reassembler, tp_segmenter, e2e_profile_registry, event_publisher)Test plan
Ref: #222
Made with Cursor
Summary by CodeRabbit
Chores
Refactor