fix: remediate batch 5 clang-tidy violations#230
Conversation
Signed-bitwise (hicpp-signed-bitwise, ~349 fixes): - Cast uint8_t/uint16_t to uint32_t before bitwise shifts to prevent UB from signed int promotion - Add U suffix to hex literals used with bitwise operators - Fix across all modules: sd_message, types.h, e2e_crc, tp_segmenter, tp_reassembler, serializer, tcp_transport, event_subscriber, net_impl (posix/zephyr/lwip) Explicit conversions (5 fixes): - Add explicit to single-argument Pimpl constructors Qualified auto (3 fixes): - Use const auto* for pointer declarations in serializer Error handling (1 fix): - Replace atoi with strtol in endpoint validation 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 52 minutes and 11 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 (3)
📝 WalkthroughWalkthroughThe PR systematically adds explicit type casting, unsigned literal suffixes (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
🤖 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/e2e/e2e_profiles/standard_profile.cpp`:
- Line 269: The code narrows config.freshness_timeout_ms (uint32_t) to uint16_t
without validation, risking truncation before computing freshness_diff; add a
bounds check on config.freshness_timeout_ms at the point where timeout_units is
computed (or earlier during E2EConfig init) and handle out-of-range values
explicitly (e.g., clamp to 0xFFFF, return/log an error, or reject the config)
before casting to uint16_t; ensure the check uses the same constant 0xFFFFU and
update the logic around freshness_diff/timeout_units to rely on the validated
value (symbols to update: config.freshness_timeout_ms, timeout_units,
freshness_diff).
In `@src/tp/tp_segmenter.cpp`:
- Around line 160-164: The TP header builder uses a uint16_t offset which can
overflow for messages >65,535 bytes; update the offset/payload_offset parameter
type in serialize_tp_header (and its declaration in include/tp/tp_segmenter.h)
from uint16_t to uint32_t, adjust any local counters (e.g.,
payload_offset/counter variables used in segment loop) to uint32_t, ensure
offset_units is computed from that uint32_t and masked/shifted into the 32-bit
tp_header safely (preserve the (offset_units << 4) | flags construction but
operate on 32-bit values and cast bytes when pushing into segment_data), and
re-evaluate the 28-bit overflow check logic so it now works against the uint32_t
offset type.
🪄 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: bf94f11b-1112-49b4-b49c-beb032fda4c3
📒 Files selected for processing (19)
include/platform/lwip/net_impl.hinclude/platform/posix/net_impl.hinclude/platform/zephyr/net_impl.hinclude/sd/sd_message.hinclude/someip/types.hsrc/e2e/e2e_crc.cppsrc/e2e/e2e_profiles/standard_profile.cppsrc/events/event_subscriber.cppsrc/rpc/rpc_client.cppsrc/rpc/rpc_server.cppsrc/sd/sd_client.cppsrc/sd/sd_message.cppsrc/sd/sd_server.cppsrc/serialization/serializer.cppsrc/tp/tp_manager.cppsrc/tp/tp_reassembler.cppsrc/tp/tp_segmenter.cppsrc/transport/endpoint.cppsrc/transport/tcp_transport.cpp
- Clamp freshness_timeout_ms to 0xFFFF before narrowing to uint16_t to prevent silent truncation for large timeout values - Widen payload_offset and serialize_tp_header offset parameter from uint16_t to uint32_t to support messages larger than 65535 bytes (the whole purpose of TP segmentation) Made-with: Cursor
Summary
Fifth batch of clang-tidy remediations eliminating the largest remaining violation category — signed bitwise operations — across 19 files.
Changes by category
Signed bitwise (
hicpp-signed-bitwise, ~349 fixes)uint8_t/uint16_ttouint32_tbefore bitwise shifts to prevent UB from signedintpromotionUsuffix to hex literals used with bitwise operators (0xFF→0xFFU, etc.)Explicit conversions (5 fixes)
hicpp-explicit-conversions: Addexplicitto single-argument Pimpl constructors (event_subscriber, rpc_client, rpc_server, sd_client, sd_server)Qualified auto (3 fixes)
readability-qualified-auto: Useconst auto*for pointer declarations in serializerError handling (1 fix)
cert-err34-c: Replaceatoiwithstrtolin endpoint IPv4 validationTest plan
Ref: #222
Made with Cursor
Summary by CodeRabbit
explicitkeyword.