refactor: S10.16 Senders conformance#426
Merged
Merged
Conversation
cppcheck-misra reports rule 11.3 / 11.5 / 5.7 at the cast-expression / opening-brace line, not at the closing `}` or first enum constant. The senders-cluster suppressions had drifted one line during the E11 three-TU split. SwitchingSender:59 happens to match by coincidence (its cast and closing brace are on the same source line). No behaviour change; suppressions only.
cppcheck-misra fires both 2.4 (unused tag) and 5.7 (non-unique tag) on the same anonymous-enum named-constant idiom. The two findings share one syntactic shape and one rationale, so they belong under one deviation. D.009 now documents both rules. The three 5.7-on-anonymous-enum suppressions in the senders scope (UdpPayload.h, UdpPayload.c, StreamSender.c) move from the D.003 block (struct-tag repetition) to the D.009 block. D.003's wording stays narrowly about struct tags. Other historical 5.7-on-anonymous-enum sites in the D.003 block will migrate to D.009 as S10.17 / S10.18 / S10.19 review their clusters. The deviation that authorises each line is determined by the kind of identifier it lands on, not by the physical block — both blocks correctly authorise their respective sites during the transition.
…dary Introduce a local `trimmed` shadowing the `length` parameter so the parameter itself is not modified. Same logic, satisfies MISRA 17.8 (function parameter should not be modified). This is the senders-cluster site flagged by the S10.05 audit; the three sibling 17.8 sites in SolidSyslogFormatter.c will be fixed in S10.19.
Each null-object's singleton `instance` was declared at file scope but only referenced inside its `_Get()` accessor. MISRA 8.9 (an object should be defined at block scope if its identifier only appears in a single function) is satisfied by moving the declaration inside the accessor as a function-scope `static`. Lifetime and address stability are identical — `static` inside a function still has program-duration storage and a fixed address, so callers continue to receive the same pointer on every call. Twelve sites swept tree-wide: - NullAtomicCounter, NullBlockDevice, NullBuffer, NullDatagram, NullFile, NullMutex, NullResolver, NullSd, NullSecurityPolicy, NullSender, NullStore, NullStream. S10.16 scope is Senders only; the other eleven sister null-objects are picked up here because the finding-fix-when-we-see-it strategy costs less than splitting it across S10.17 / S10.18 / S10.19. No new deviation is needed — MISRA 8.9 is now satisfied across the whole null-object family.
…enum
cppcheck-misra reports rule 2.4 ("unused tag") at the `{` opening line
of an anonymous enum (consistent with how it reports 5.7), not at the
first constant. The StreamSender suppression was anchored at line 20
(the first constant) and worked only because the matching 5.7
suppression on the same site shadowed it via cppcheck's dedup. Once
S10.16 commit ca76268 corrected the 5.7 anchor to line 19, the 2.4
finding surfaced.
Bumping the 2.4 suppression line :20 → :19. Sister anonymous-enum
2.4 suppressions (BlockSequence.c:13, RecordStore.c:14,
Transport.h:5) already match this convention.
Records the per-group review outcome, the two decisions taken at the bend (sweep all 12 null-objects, widen D.009 to 2.4 + 5.7), and the cppcheck dedup follow-up that surfaced a second anchor drift on the StreamSender anonymous enum.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughNull-component getter functions move singleton instances from file scope to function-local scope across twelve implementations. UDP payload trimming refactors parameter handling to use local variables. DEVLOG, deviation documentation, and suppression tracking are updated to record conformance closure and line-number adjustments. ChangesS10.16 Senders Conformance Closure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1296 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
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.
Purpose
Closes #389. Fifth per-group conformance story in E10, applying the S10.12 pilot recipe to the Senders cluster (
Sender,NullSender,UdpSender,StreamSender,SwitchingSender,UdpPayload, plus the E11 three-TU split files for the three pool-backed senders). The aim is zero in-scope cppcheck-misra and clang-tidy findings after the work, with each existing suppression re-justified per-site and any genuine local defect fixed instead of carried as a deviation.Change Description
Started from the CI cppcheck-misra report against
main@ddbc81f(run26293233352).analyze-tidywas already clean against the cluster; all work was on the cppcheck-misra side. Six unsuppressed findings split into:misra_suppressions.txt. The 11.3 / 11.5 vtable-cast suppressions were anchored at the closing}of the inlineSelfFromBase(or the comment line for 11.5) instead of the cast-expression line cppcheck reports against. The 5.7 anchor on the StreamSender anonymous enum was at the first constant rather than the{.SwitchingSender:59matched by coincidence — its cast and brace are on the same line. One-line bumps.UdpPayload.c:37: thelengthparameter was being mutated in-place. Introduced a localtrimmedcopy and return that instead. The audit anticipated this site; the three sibling 17.8 sites inSolidSyslogFormatter.cbelong to S10.19.NullSender.c:11flagged the file-scopestatic struct SolidSyslogSender instance = {...}as only-used-by-_Get(). The same finding fires on all 12 sister null-objects (NullBuffer, NullStore, NullDatagram, NullStream, NullSd, NullMutex, NullSecurityPolicy, NullAtomicCounter, NullBlockDevice, NullFile, NullResolver, plus NullSender). None were previously suppressed, so the cluster was known-pending. Discussed with David — chose to sweep all 12 in this story (fix-when-we-see-it) rather than spread it across S10.17–S10.19. The instance moves inside the accessor as a function-scopestatic; lifetime and address stability are identical. No new deviation.UdpPayload.h:15,StreamSender.c:19,UdpPayload.c:5) migrated from the D.003 block (struct-tag repetition) to the D.009 block. Other historical 5.7-on-anonymous-enum suppressions stay in D.003 until S10.17 / S10.18 / S10.19 review their clusters — D.009's wording now states that the deviation authorising each 5.7 line is determined by the identifier kind, not by the physical block.StreamSender.c:19, a 2.4 finding surfaced on the same line — previously masked by the matching (mis-anchored) 5.7. The existing 2.4 suppression at:20bumped to:19. Worth noting for the rest of E10: removing or moving one suppression can unmask another on the same source line.Six commits on the branch — one per logical cluster. Full per-site notes in the DEVLOG entry on this branch.
Test Evidence
debugbuild clean.sanitize(ASan + UBSan) clean; 1290 / 1290 tests pass.tidypreset clean (zero in-scope warnings, unchanged from starting state).cppcheck-misra(with--suppressions-list=misra_suppressions.txt) clean — zero in-scope findings; verified against the sameghcr.io/davidcozens/cpputest:sha-18f19e1image the CI uses.coverage99.9% (2921 / 2925 lines, 602 / 602 functions). The four uncovered lines sit inBlockStoreStatic.candPosixMutex.c— pre-existing, not in scope, no changed file lost coverage.clang-formatclean tree-wide.Areas Affected
Core/Source/SolidSyslogUdpPayload.c— production fix for rule 17.8 (local copy of thelengthparameter).Core/Source/SolidSyslogNull*.c(12 files) — production refactor for rule 8.9 (file-scopeinstancemoved into_Get()as a function-scopestatic). No semantic change.misra_suppressions.txt— five anchor bumps; three 5.7 lines migrated from the D.003 block to the D.009 block; one comment header updated.docs/misra-deviations.md— D.009 title and rule statement widened to cover rules 2.4 + 5.7; a new "suppression-file layout" note explains the in-progress migration.DEVLOG.md— entry recording the per-site decisions and the dedup follow-up.No public-API changes. Downstream callers of
SolidSyslogNullSender_Get()(and sister null-object accessors) get the exact same pointer they did before — block-scopestaticwith a constant initializer is identical to file-scopestaticin lifetime and address.Summary by CodeRabbit
Refactor
Documentation