refactor: S10.13 Security policies + CRC + Sync primitives conformance#384
Conversation
Three new findings surfaced by cppcheck-misra in the S10.13 group: - NullMutex.c:10 (5.9) — bare `instance` collides with same-named statics in NullSecurityPolicy.c and Crc16Policy.c. Renamed to `NullMutex_Instance` per the S10.08 / S10.12 `<Class>_Instance` convention. Can't move into _Create because _Destroy also touches it. - NullSecurityPolicy.c:24 (8.9) — file-scope `static instance` only read in _Create. Moved into _Create as a block-scope static. - Crc16Policy.c:17 (8.9) — same pattern, same fix. No new deviations, no new cppcheck-suppress comments. Full-tree cppcheck-misra clean on the three files post-edit; clang-tidy clean tree-wide; 1122 unit tests pass.
Three small changes reviewed point-by-point in the CRC-16 inner loop: - `int bit` → `uint_fast8_t bit` — match the comparand's unsigned essential type. cppcheck-misra's chosen 10.4 subset doesn't flag the signed/unsigned mix in for-loop bounds, but it's still the right type. - `!= 0` → `!= 0U` — match the U-suffixed literal style used elsewhere in the file (S10.10 sweep convention). - Drop the `BITS_PER_BYTE = 8U` enum constant; use literal `8U` inline. The named constant was used in exactly one place — the bit-iteration loop — and its meaning is self-evident from the surrounding CRC code. Dropping it also sidesteps clang-tidy's bugprone-too-small-loop-variable warning that fired when the new `uint_fast8_t` loop variable was compared against an anonymous-enum constant typed as `int`. The other enum constants (CRC16_CCITT_INIT / CRC16_CCITT_POLY / MSB_MASK) carry non-obvious values and retain their names. Gates: tidy clean, cppcheck-misra clean on S10.13 scope, 1122 tests pass.
📝 WalkthroughWalkthroughThis PR addresses MISRA conformance findings in the security policies, CRC, and sync primitives group by refining loop variable types, relocating static instances into factory functions, and explicitly managing function pointer lifecycles across create/destroy operations. ChangesMISRA Conformance: Security Policies, CRC, and Sync Primitives
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@DEVLOG.md`:
- Around line 52-53: Update the DEVLOG note that currently calls uint_fast8_t
"8-bit" to state it is "at least 8 bits" (or similar wording) so it reflects the
C guarantee that uint_fast8_t may be wider on many targets; locate the sentence
mentioning uint_fast8_t in the paragraph around the existing text and replace
"8-bit `uint_fast8_t`" with "at least 8-bit `uint_fast8_t`" or "minimum 8-bit
`uint_fast8_t`" to clarify the guarantee.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 837a74d9-a35f-4031-b2eb-905fc4576b21
📒 Files selected for processing (5)
Core/Source/SolidSyslogCrc16.cCore/Source/SolidSyslogCrc16Policy.cCore/Source/SolidSyslogNullMutex.cCore/Source/SolidSyslogNullSecurityPolicy.cDEVLOG.md
| their literal. clang-tidy compared the 8-bit `uint_fast8_t` | ||
| variable against the 32-bit `int` bound and flagged it. Three fixes |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C99/C11/C17, is uint_fast8_t guaranteed to be exactly 8 bits, or only at least 8 bits?
💡 Result:
In C99, C11, and C17, the type uint_fast8_t is guaranteed to be an unsigned integer type with a width of at least 8 bits [1][2][3]. It is not guaranteed to be exactly 8 bits [4][5]. The C standard defines uint_fastN_t as the fastest unsigned integer type available on the implementation that has a width of at least N bits [1][3]. Because the implementation may choose a larger type (such as 16, 32, or 64 bits) if it is more efficient for the target architecture's word size or alignment requirements, uint_fast8_t can be wider than 8 bits [4][5]. In contrast, the type uint8_t is defined as an unsigned integer type with a width of exactly 8 bits, but it is an optional type and is not guaranteed to be provided by all implementations [2][6]. If an implementation does not support an 8-bit type, uint8_t will not be defined, whereas uint_fast8_t and uint_least8_t are required to be provided [1][4][3].
Citations:
- 1: https://pubs.opengroup.org/onlinepubs/9699919799.2016edition/basedefs/stdint.h.html
- 2: https://en.cppreference.com/c/types/integer
- 3: https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/basedefs/stdint.h.html
- 4: https://stackoverflow.com/questions/35055042/difference-between-uint8-t-uint-fast8-t-and-uint-least8-t
- 5: https://www.geeksforgeeks.org/cpp/extended-integral-types-choosing-correct-integer-size-cc/
- 6: https://en.cppreference.com/c/header/stdint
🏁 Script executed:
head -60 DEVLOG.md | tail -20Repository: DavidCozens/solid-syslog
Length of output: 1010
Correct uint_fast8_t description from "8-bit" to at least 8 bits.
At lines 52–53, the note describes uint_fast8_t as "8-bit". In C, uint_fast8_t is guaranteed to be at least 8 bits and may be wider (16, 32, or 64 bits) on many targets. The phrasing is misleading in technical documentation.
Consider clarifying:
Proposed revision
- their literal. clang-tidy compared the 8-bit `uint_fast8_t`
- variable against the 32-bit `int` bound and flagged it. Three fixes
+ their literal. clang-tidy compared the `uint_fast8_t` variable
+ (which may be wider than 8 bits depending on the target) against
+ the `int`-typed bound and flagged it. Three fixes📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| their literal. clang-tidy compared the 8-bit `uint_fast8_t` | |
| variable against the 32-bit `int` bound and flagged it. Three fixes | |
| their literal. clang-tidy compared the `uint_fast8_t` variable | |
| (which may be wider than 8 bits depending on the target) against | |
| the `int`-typed bound and flagged it. Three fixes |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DEVLOG.md` around lines 52 - 53, Update the DEVLOG note that currently calls
uint_fast8_t "8-bit" to state it is "at least 8 bits" (or similar wording) so it
reflects the C guarantee that uint_fast8_t may be wider on many targets; locate
the sentence mentioning uint_fast8_t in the paragraph around the existing text
and replace "8-bit `uint_fast8_t`" with "at least 8-bit `uint_fast8_t`" or
"minimum 8-bit `uint_fast8_t`" to clarify the guarantee.
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1126 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Purpose
Second per-group conformance story in E10, applying the S10.12 pilot recipe to the Security policies + CRC + Sync primitives group. Resolves all warning-mode findings raised by
analyze-tidyandanalyze-cppcheck(with the cppcheck-misra addon) against the files listed under Areas Affected.Closes #383
Change Description
MISRA fixes — 3 new findings, each reviewed per-site
Core/Source/SolidSyslogNullMutex.c:10(bareinstancecollided with same name in NullSecurityPolicy and Crc16Policy)NullMutex_Instanceper the S10.08 / S10.12<Class>_Instanceconvention. Couldn't move into_Createbecause_Destroyalso touches it.Core/Source/SolidSyslogNullSecurityPolicy.c:24(file-scopestatic instanceonly read in_Create)_Createas a block-scope static, matching the S10.12QUEUE_NAME_PREFIXrecipe.Core/Source/SolidSyslogCrc16Policy.c:17(same pattern)_Create.No new deviations. No new
cppcheck-suppressinline comments. No count bumps to existing deviations (D.002 11.3 mutex casts / D.003 5.7 tag-name / D.010 2.4 anonymous-enum) — no new sites accrued.CRC-16 inner-loop review
SolidSyslogCrc16.cwas already MISRA-clean, so this was a deliberate "walk the loop and decide whether to tighten anything anyway" pass. Three small changes:int bit→uint_fast8_t bit— match the comparand's unsigned essential type.!= 0→!= 0U— U-suffixed literal style (S10.10 convention).BITS_PER_BYTE = 8Uenum constant; inlined the literal.The last one needed thinking — switching the loop variable to
uint_fast8_tunmasked clang-tidy'sbugprone-too-small-loop-variable, because anonymous-enum constants in C have essential typeintregardless of theUsuffix. clang-tidy compared the 8-bit loop variable against the 32-bitintenum bound and flagged it. Three options weighed (inline the literal; promote the constant tostatic const uint8_t; widen the loop variable tounsigned) — chose inline-the-literal becauseBITS_PER_BYTEwas a one-use named constant whose meaning is self-evident from the surrounding CRC code. The other CRC enum constants (CRC16_CCITT_INIT,CRC16_CCITT_POLY,MSB_MASK) carry non-obvious values and retain their names.Full discussion in
DEVLOG.md.Test Evidence
This is MISRA-conformance refactoring with no behaviour change — the bar is "existing tests stay green, gates clean".
Ran locally under the dev container:
cmake --preset debug && cmake --build --preset debug --target junit→ 1122 tests, 0 failurescmake --preset clang-debug && cmake --build --preset clang-debug --target junit→ clean, 1122 rancmake --preset sanitize && cmake --build --preset sanitize --target junit→ clean, 1122 rancmake --preset tidy && cmake --build --preset tidy→ 0 findingscmake --preset coverage && cmake --build --preset coverage --target coverage→ 100% line coverage on every Linux-buildable file in scopecppcheck Core/Source/→ clean (0 findings)cppcheck --addon=misra→ zero findings on S10.13 scope filesclang-format --dry-run --Werroron edited files → cleanWindows / BDD / OpenSSL integration jobs are CI's responsibility per the CLAUDE.md workflow note.
Areas Affected
Edited:
Core/Source/SolidSyslogCrc16.cCore/Source/SolidSyslogCrc16Policy.cCore/Source/SolidSyslogNullMutex.cCore/Source/SolidSyslogNullSecurityPolicy.cDEVLOG.mdIn scope but already clean (no edits required):
Core/Interface/SolidSyslogSecurityPolicyDefinition.hCore/Interface/SolidSyslogNullSecurityPolicy.hCore/Interface/SolidSyslogCrc16.hCore/Interface/SolidSyslogCrc16Policy.hCore/Interface/SolidSyslogMutex.hCore/Interface/SolidSyslogMutexDefinition.hCore/Interface/SolidSyslogNullMutex.hCore/Source/SolidSyslogMutex.cPlatform/Posix/Interface/SolidSyslogPosixMutex.hPlatform/Posix/Source/SolidSyslogPosixMutex.cPlatform/Windows/Interface/SolidSyslogWindowsMutex.hPlatform/Windows/Source/SolidSyslogWindowsMutex.cPlatform/FreeRtos/Interface/SolidSyslogFreeRtosMutex.hPlatform/FreeRtos/Source/SolidSyslogFreeRtosMutex.cSummary by CodeRabbit
Refactor
Documentation