fix: remediate batch 2 clang-tidy violations#227
Merged
Conversation
Performance: - Use std::move for callback parameters instead of copying - Replace std::endl with '\n' to avoid unnecessary flushes - Pass MessagePtr by const ref where only read access is needed Readability: - Access static members via class name, not instance - Simplify boolean returns in has_valid_method_id/has_valid_length - Remove redundant member initializer in UdpConfig - Use const auto* for pointer-to-const auto declarations - Initialize TpTransfer::last_activity in member initializer list Misc: - Move file-local static variables/functions to anonymous namespaces - Replace C-style casts with reinterpret_cast in socket code Tests: - Add boundary tests for method ID and length validation - Add E2E header size static access test - Add TpTransfer timestamp initialization test Made-with: Cursor
Contributor
📝 WalkthroughWalkthroughRefactors E2E header size access to use static Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 docstrings
🧪 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 |
Made-with: Cursor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (5)
src/e2e/e2e_crc.cpp (1)
80-103:⚠️ Potential issue | 🔴 CriticalMake CRC32 table initialization thread-safe.
The unsynchronized shared mutable state at lines 80 and 83 (
crc32_tableandcrc32_table_initialized) creates a race condition. Concurrent calls tocalculate_crc32()can simultaneously invokeinit_crc32_table(), causing partial writes tocrc32_tableand nondeterministic CRC values.Replace the mutable globals and
init_crc32_table()function with a thread-safe static local initialization:Proposed fix
+#include <array> ... namespace { - -// Precomputed CRC32 lookup table -uint32_t crc32_table[256]; - -// Initialize CRC32 lookup table (called once) -bool crc32_table_initialized = false; - -void init_crc32_table() { - if (crc32_table_initialized) { - return; - } - - for (uint32_t i = 0; i < 256; ++i) { - uint32_t crc = i << 24; - for (int j = 0; j < 8; ++j) { - if (crc & 0x80000000) { - crc = (crc << 1) ^ CRC32_POLY; - } else { - crc <<= 1; - } - } - crc32_table[i] = crc; - } - - crc32_table_initialized = true; -} +const std::array<uint32_t, 256>& get_crc32_table() { + static const std::array<uint32_t, 256> table = [] { + std::array<uint32_t, 256> t{}; + for (uint32_t i = 0; i < 256; ++i) { + uint32_t crc = i << 24; + for (int j = 0; j < 8; ++j) { + if (crc & 0x80000000) { + crc = (crc << 1) ^ CRC32_POLY; + } else { + crc <<= 1; + } + } + t[i] = crc; + } + return t; + }(); + return table; +} } // namespace uint32_t calculate_crc32(const std::vector<uint8_t>& data) { - init_crc32_table(); + const auto& crc32_table = get_crc32_table(); uint32_t crc = CRC32_INIT; for (uint8_t byte : data) { uint32_t index = ((crc >> 24) ^ byte) & 0xFF; crc = (crc << 8) ^ crc32_table[index]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e/e2e_crc.cpp` around lines 80 - 103, Replace the global mutable crc32_table and crc32_table_initialized and the init_crc32_table() function with a thread-safe static local initialization so concurrent calls to calculate_crc32() cannot race; specifically, remove the globals and have calculate_crc32() (or a helper it calls) declare a static local std::array<uint32_t,256> (or equivalent) that is populated the first time via the same loop, relying on C++ static local initialization to be thread-safe, and update references that used crc32_table to use that static local variable instead.src/events/event_subscriber.cpp (2)
92-101: 🧹 Nitpick | 🔵 TrivialMove
SubscriptionInfointo the map too.Lines 94-95 move the callbacks into
sub_info, but Line 101 storessub_infoas an lvalue, so bothstd::functionobjects are still copied once. Finish the handoff with a move here as well.Proposed fix
- subscriptions_[key] = sub_info; + subscriptions_[key] = std::move(sub_info);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/event_subscriber.cpp` around lines 92 - 101, The SubscriptionInfo object `sub_info` currently has its `notification_callback` and `status_callback` moved into it but is then stored into `subscriptions_` as an lvalue, causing another copy of the std::function objects; change the store to move `sub_info` into the map (e.g., use `subscriptions_.emplace(key, std::move(sub_info))` or `subscriptions_[key] = std::move(sub_info)`) so the callbacks are fully moved and not copied again; update the code around `make_subscription_key`, `sub_info`, and `subscriptions_` accordingly while keeping the `ScopedLock` usage.
170-188:⚠️ Potential issue | 🔴 CriticalField requests are registered under a key that the receive path never uses.
Lines 172-173 store callbacks under
(service_id, instance_id, event_id), buton_message_received()looks them up with(service_id, 0, event_id)at Line 292. For any nonzeroinstance_id, the callback can never be found, and this function also leaves the orphaned entry behind whensend_message()fails at Line 188.Please use the same correlation key on both sides, or key pending field requests by request/session ID and only retain the callback after a successful send.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/event_subscriber.cpp` around lines 170 - 188, The callback is being stored under make_field_key(service_id, instance_id, event_id) but on_message_received looks up callbacks using make_field_key(service_id, 0, event_id), and the entry is left behind if send_message fails; change event_subscriber.cpp so you (a) construct the same correlation key used by on_message_received (i.e., call make_field_key(service_id, 0, event_id) instead of using instance_id) or alternatively change on_message_received to use the instance_id consistently, and (b) only insert into field_requests_ (protected by field_requests_mutex_) after transport_->send_message(...) returns Result::SUCCESS, or if you must insert before sending remove the entry on send failure to avoid orphaned callbacks; reference field_requests_mutex_, make_field_key, field_requests_, transport_->send_message, and on_message_received when making the edit.src/someip/message.cpp (2)
455-463: 🧹 Nitpick | 🔵 TrivialDrop the no-op total-size comparison.
expected_total_sizeandactual_total_sizeare identical expressions, so this branch can never fail and only hides the real check below onlength_.Suggested simplification
- uint32_t expected_total_size = HEADER_SIZE + e2e_size + payload_.size(); - uint32_t actual_total_size = HEADER_SIZE + e2e_size + payload_.size(); // Same calculation - if (expected_total_size != actual_total_size) { - return false; - } - // Also check that length_ is consistent uint32_t expected_length = 8 + e2e_size + payload_.size(); if (length_ != expected_length) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/someip/message.cpp` around lines 455 - 463, The current no-op compares two identical expressions (expected_total_size and actual_total_size); remove the duplicate actual_total_size and instead validate the parsed length field: compute e2e_size using e2e_header_.has_value() ? e2e::E2EHeader::get_header_size() : 0, set expected_total_size = HEADER_SIZE + e2e_size + payload_.size(), and compare expected_total_size against the message length field (length_) so the real consistency check is performed (replace the redundant comparison with: if (expected_total_size != length_) return false;).
115-126:⚠️ Potential issue | 🟠 MajorPreserve
interface_version_in move assignment.Line 121 resets the destination to
SOMEIP_INTERFACE_VERSIONinstead of takingother.interface_version_, breaking consistency with the move constructor (which preserves the field) and copy assignment (which also preserves it). The destination should receive the source's state, not a reset constant.Proposed fix
- interface_version_ = SOMEIP_INTERFACE_VERSION; // Valid interface version for moved-to object + interface_version_ = other.interface_version_;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/someip/message.cpp` around lines 115 - 126, The move-assignment operator Message::operator=(Message&& other) currently resets interface_version_ to SOMEIP_INTERFACE_VERSION instead of preserving the moved-from value; change it to assign other.interface_version_ so the destination receives the source's state (matching the move constructor and copy assignment), i.e., replace the line that sets interface_version_ with an assignment from other.interface_version_, keeping the rest of the move logic (payload_, e2e_header_, etc.) unchanged.
🤖 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/platform/freertos/memory.cpp`:
- Around line 67-75: Validate pool ownership before destroying the Message: in
release_message (the function calling msg->~Message()), include <cstdint>,
compute uintptr_t raw = reinterpret_cast<std::uintptr_t>(msg) and uintptr_t pool
= reinterpret_cast<std::uintptr_t>(pool_buffer), check alignment and that raw >=
pool and (raw - pool) % sizeof(someip::Message) == 0 and index = (raw - pool) /
sizeof(someip::Message) is < POOL_SIZE; only after these checks take pool_mutex,
mark block_used[index] = false and then call the destructor (msg->~Message());
do not perform pointer subtraction or call the destructor before verifying msg
belongs to this pool.
In `@tests/test_tp.cpp`:
- Around line 691-701: The test TransferInitTimestamps references a non-existent
field transfer.total_size on TpTransfer; change the assertion to check the
actual field name message_id instead (i.e., replace
EXPECT_EQ(transfer.total_size, 1400u) with an assertion that
EXPECT_EQ(transfer.message_id, 1400u)) so the test compiles and verifies the
intended value; keep the other assertions (transfer_id, start_time,
last_activity) unchanged.
---
Outside diff comments:
In `@src/e2e/e2e_crc.cpp`:
- Around line 80-103: Replace the global mutable crc32_table and
crc32_table_initialized and the init_crc32_table() function with a thread-safe
static local initialization so concurrent calls to calculate_crc32() cannot
race; specifically, remove the globals and have calculate_crc32() (or a helper
it calls) declare a static local std::array<uint32_t,256> (or equivalent) that
is populated the first time via the same loop, relying on C++ static local
initialization to be thread-safe, and update references that used crc32_table to
use that static local variable instead.
In `@src/events/event_subscriber.cpp`:
- Around line 92-101: The SubscriptionInfo object `sub_info` currently has its
`notification_callback` and `status_callback` moved into it but is then stored
into `subscriptions_` as an lvalue, causing another copy of the std::function
objects; change the store to move `sub_info` into the map (e.g., use
`subscriptions_.emplace(key, std::move(sub_info))` or `subscriptions_[key] =
std::move(sub_info)`) so the callbacks are fully moved and not copied again;
update the code around `make_subscription_key`, `sub_info`, and `subscriptions_`
accordingly while keeping the `ScopedLock` usage.
- Around line 170-188: The callback is being stored under
make_field_key(service_id, instance_id, event_id) but on_message_received looks
up callbacks using make_field_key(service_id, 0, event_id), and the entry is
left behind if send_message fails; change event_subscriber.cpp so you (a)
construct the same correlation key used by on_message_received (i.e., call
make_field_key(service_id, 0, event_id) instead of using instance_id) or
alternatively change on_message_received to use the instance_id consistently,
and (b) only insert into field_requests_ (protected by field_requests_mutex_)
after transport_->send_message(...) returns Result::SUCCESS, or if you must
insert before sending remove the entry on send failure to avoid orphaned
callbacks; reference field_requests_mutex_, make_field_key, field_requests_,
transport_->send_message, and on_message_received when making the edit.
In `@src/someip/message.cpp`:
- Around line 455-463: The current no-op compares two identical expressions
(expected_total_size and actual_total_size); remove the duplicate
actual_total_size and instead validate the parsed length field: compute e2e_size
using e2e_header_.has_value() ? e2e::E2EHeader::get_header_size() : 0, set
expected_total_size = HEADER_SIZE + e2e_size + payload_.size(), and compare
expected_total_size against the message length field (length_) so the real
consistency check is performed (replace the redundant comparison with: if
(expected_total_size != length_) return false;).
- Around line 115-126: The move-assignment operator Message::operator=(Message&&
other) currently resets interface_version_ to SOMEIP_INTERFACE_VERSION instead
of preserving the moved-from value; change it to assign other.interface_version_
so the destination receives the source's state (matching the move constructor
and copy assignment), i.e., replace the line that sets interface_version_ with
an assignment from other.interface_version_, keeping the rest of the move logic
(payload_, e2e_header_, etc.) unchanged.
🪄 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: 588b8f03-f379-48cf-abe1-4e07cbd430f1
📒 Files selected for processing (17)
include/someip/message.hinclude/tp/tp_types.hinclude/transport/udp_transport.hsrc/e2e/e2e_crc.cppsrc/events/event_subscriber.cppsrc/platform/freertos/memory.cppsrc/platform/threadx/memory.cppsrc/rpc/rpc_client.cppsrc/rpc/rpc_server.cppsrc/sd/sd_client.cppsrc/sd/sd_message.cppsrc/sd/sd_server.cppsrc/someip/message.cppsrc/tp/tp_reassembler.cppsrc/transport/tcp_transport.cpptests/test_message.cpptests/test_tp.cpp
Owner
Author
Review comment triageFixed in next push:
Pre-existing, out of scope for this PR:
|
- Validate FreeRTOS pool ownership before destroying Message (bounds + alignment check prevents UB on foreign pointers) - Replace CRC32 mutable globals with thread-safe static local initialization (C++ magic statics guarantee thread safety) - Move SubscriptionInfo into map instead of copying - Fix move assignment to preserve interface_version_ from source - Remove no-op identical comparison in has_valid_header - Add move assignment test for interface_version preservation Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/platform/freertos/memory.cpp`:
- Around line 67-76: Replace the pointer relational checks on raw and pool_start
with integer comparisons using std::uintptr_t: include <cstdint>, cast raw and
pool_start to std::uintptr_t (e.g., uintptr_t raw_addr =
reinterpret_cast<uintptr_t>(raw); uintptr_t pool_addr =
reinterpret_cast<uintptr_t>(pool_start);) and then perform bounds checks against
pool_addr and pool_addr + POOL_SIZE * sizeof(someip::Message); keep the existing
byte_offset/modulo/index logic unchanged after the validated cast.
🪄 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: 12a7b25e-314e-41eb-affe-296a944e23dc
📒 Files selected for processing (5)
src/e2e/e2e_crc.cppsrc/events/event_subscriber.cppsrc/platform/freertos/memory.cppsrc/someip/message.cpptests/test_message.cpp
vtz
added a commit
that referenced
this pull request
Apr 13, 2026
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
3 tasks
vtz
added a commit
that referenced
this pull request
Apr 13, 2026
* fix(freertos): use std::uintptr_t for pool ownership validation 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) * fix: remediate batch 3 clang-tidy violations 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) * fix(tp): address CodeRabbit review findings - 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
5 tasks
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
1 participant
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
Second batch of clang-tidy remediations targeting performance, readability, and code organization violations across 15 source files.
Changes by category
Performance (17 fixes)
performance-unnecessary-value-param: Usestd::movefor callback parameters instead of copying (event_subscriber, rpc_client, rpc_server)performance-avoid-endl: Replacestd::endlwith'\n'(sd_message, tp_reassembler)MessagePtrbyconst&in rpc_server private helpers where only read access is neededReadability (28 fixes)
readability-static-accessed-through-instance: AccessE2EHeader::get_header_size()via class name (message.h, message.cpp)readability-simplify-boolean-expr: Simplifyhas_valid_method_id()andhas_valid_length()returnsreadability-redundant-member-init: Remove redundant{}onUdpConfig::multicast_interfacereadability-qualified-auto: Useconst auto*for pointer-to-const (sd_client, sd_server)cppcoreguidelines-prefer-member-initializer: MoveTpTransfer::last_activityto initializer listMisc / Organization (17 fixes)
misc-use-anonymous-namespace: Move file-local statics to anonymous namespaces (e2e_crc, freertos/memory, threadx/memory, sd_client, sd_server)cppcoreguidelines-pro-type-cstyle-cast: Replace C-style casts withreinterpret_castin socket code (tcp_transport)Tests added
has_valid_method_id()(0x0000, 0x0001, 0xFFFE, 0xFFFF)has_valid_length()(0, 7, 8, 9, UINT32_MAX)E2EHeader::get_header_size()static access verificationTpTransfertimestamp initialization consistency testTest plan
Ref: #222
Made with Cursor
Summary by CodeRabbit
Refactor
Stability
Tests