feat: S11.01 CircularBuffer pool migration (E11 pilot)#394
Merged
Conversation
…r-supplied Splits the previously-conflated CircularBuffer storage. The instance bookkeeping struct (vtable, mutex pointer, ring pointer, head/tail/wrap) now lives in a library-internal static pool; the caller continues to supply the ring memory as plain uint8_t* + size. This addresses the original E11 driver — eliminating the void* storage cast on the instance — without conflating it with a tunable for ring capacity (which would not work: different call sites need different ring sizes inside the same build). Public API change: - SolidSyslogCircularBuffer_Create(mutex, ring, ringBytes) replaces Create(storage, storageBytes, mutex). - SolidSyslogCircularBufferStorage typedef, OVERHEAD constant, and STORAGE_SIZE / STORAGE_SIZE_BYTES macros removed. - New convenience macro SOLIDSYSLOG_CIRCULAR_BUFFER_RING_BYTES(maxMessages) for sizing the caller's ring. - HEADER_BYTES stays public — needed by the new macro. Three-TU split (E11 canonical pattern): - SolidSyslogCircularBuffer.c: vtable + ring logic. Exposes CircularBuffer_Initialise(self, mutex, ring, ringBytes) and CircularBuffer_Cleanup(self) via SolidSyslogCircularBufferPrivate.h. - SolidSyslogCircularBufferPrivate.h: struct definition + helper signatures. TU-local; tests must not include it. - SolidSyslogCircularBufferStatic.c: 1-slot static pool, public Create/Destroy. Next commit grows pool size, adds the fallback object, wraps slot walks in SolidSyslog_LockConfig/UnlockConfig, and handles unknown-pointer Destroy. CMake gate (E11): - SOLIDSYSLOG_ALLOCATION_STRATEGY cache var (static|dynamic, default static). dynamic errors with FATAL_ERROR — no dynamic TUs ship yet. MISRA / static analysis: - Storage-cast suppression (CircularBuffer.c:72 SelfFromStorage) deleted — function gone. - 18.7 FAM suppression (CircularBuffer.c:26) deleted — struct no longer has a flexible array member; ring is now an external pointer. - 2.5 STORAGE_SIZE macro suppression deleted — macro gone, RING_BYTES takes the remaining slot. - 5.7 + 2.4 + 11.3 (SelfFromBase) + 2.4 + 5.7 (anonymous enums) suppression line numbers updated. - One new constParameter inline suppression in Static.c for the transitional 1-slot Destroy that only compares the base pointer; next commit's pool walk + Cleanup wiring mutates through it and the suppression goes away. - Net unsuppressed cppcheck-misra findings 93 -> 87 across the scanned Core+Platform tree. Doc updates: - CLAUDE.md public-header audience table: row rewritten for new API. - README.md public-headers list: bullet updated to call out caller supplies ring memory only, instance lives in static pool. - docs/NAMING.md: SelfFromStorage example switched to BlockStore (still on the caller-storage pattern); added a note that E11-migrated classes (currently only CircularBuffer) no longer use the cast. - docs/misra-deviations.md: D.002 caller-storage list drops CircularBuffer with a note pointing at E11; D.005 FAM scope reduces to Formatter only; D.012 macro inventory updated to RING_BYTES. - docs/iec62443.md: "caller-allocated storage" -> "caller-allocated ring memory" in the RTOS-integration prose. - SKILL.md: matching prose update. Tests + BDD targets updated: - HandleEqualsStorageAddress test removed (caller no longer addresses the instance struct; only the ring is at a known address). - TEST_MAX_MESSAGES / BDD_TARGET_BUFFER_MESSAGES / etc. constants kept unchanged at every call site — only the API mechanics changed. Gates all green locally: - debug, clang-debug, sanitize: 1118/1118 tests pass. - tidy, cppcheck, cppcheck-misra: clean. - coverage: 99.6% lines / 99.0% functions. - cmake -DSOLIDSYSLOG_ALLOCATION_STRATEGY=dynamic errors as designed.
…ithm disabled project-wide Adds the second half of the pool mechanics now that there is a real consumer of POOL_SIZE: pool walks (claim + release) wrap in SolidSyslog_LockConfig / _UnlockConfig, exhausted Create returns a class-private fallback singleton with no-op vtable, and emits SolidSyslog_Error(ERROR, "...pool exhausted..."). Tunable: - SOLIDSYSLOG_CIRCULAR_BUFFER_POOL_SIZE (default 1U) added to SolidSyslogTunablesDefaults.h with the standard #ifndef / floor guard pattern + NOLINTNEXTLINE matching SOLIDSYSLOG_MAX_MESSAGE_SIZE. CMake / cppcheck: - Global --suppress=useStlAlgorithm added to CMAKE_CXX_CPPCHECK. The rule only ever fires on C++ test code (production is C, no STL), and replacing test loops with std::generate masks the side-effecting Create per iteration rather than expressing it. Replaces the per-loop inline suppression that the earlier attempt added. Tests: - New TEST_GROUP(SolidSyslogCircularBufferPool) covering: filling the pool returns distinct buffers + a distinct fallback on overflow, exhausted Create reports ERROR, fallback Read/Write are no-ops, Create and Destroy each wrap in LockConfig / UnlockConfig. - Tests are parameterised on SOLIDSYSLOG_CIRCULAR_BUFFER_POOL_SIZE (loop POOL_SIZE Creates into a fixture array, then one more for the overflow) so they work at any pool-size tunable. Outstanding hygiene (next commit): - Cleanup currently takes struct SolidSyslogCircularBuffer*; will change to take struct SolidSyslogBuffer* and do SelfFromBase internally so Static.c no longer needs the constParameter suppression. - Extract Pool_TryClaim / Pool_Release / Slot_OwnsBase static-inlines in Static.c to express intent and DRY the two pool walks. - Improve test diagnostic messages: bare CHECK(...) replaced with CHECK_TEXT(...) or a CHECK_IS_FALLBACK(buf, pool) macro for the central pool-test assertion.
Pure formatting pass on CircularBuffer.h / .c / Private.h. The earlier S11.01 commits left aligned-column declarations that the project's clang-format rejects (PointerAlignment: Left, AlignConsecutiveDeclarations: None). No code changes.
SolidSyslogCircularBuffer_Destroy now emits SolidSyslog_Error (SOLIDSYSLOG_SEVERITY_WARNING) when given a handle the pool never issued. Handles legitimately returned by _Create — pool-slot bases and the pool-exhaustion Fallback — remain silent. Drives the acceptance criterion from #392. Two tests in TEST_GROUP(SolidSyslogCircularBufferPool): - DestroyOfUnknownHandleReportsWarning pins the WARNING severity and message text via the existing ErrorHandlerFake. - DestroyOfFallbackHandleIsSilent pins the Fallback exception (otherwise every integrator who Destroys the buffer they were given would get a spurious warning when the pool was exhausted). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cleanup now takes struct SolidSyslogBuffer* and casts internally, matching the symmetry of every other vtable method on the class. *Static.c can hand it the integrator's handle directly, which removes the cppcheck constParameter complaint that previously needed an inline suppression on _Destroy. This is the pattern the rest of E11 will follow: Initialise takes the concrete pool slot (known shape inside *Static.c), Cleanup takes the public handle (unknown provenance from the caller's side). Adjusts the existing 11.3 line-pinned suppression for the SelfFromBase cast — the cast moved one line down inside the new Cleanup body. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_Create and _Destroy now read as the lifecycle they describe:
_Create -> LockConfig; claimed = TryClaim(...); UnlockConfig;
if (!claimed) Error+Fallback; return claimed;
_Destroy -> if (base != &Fallback) {
LockConfig; released = Release(base); UnlockConfig;
if (!released) Error(WARNING, unknown handle);
}
Three new file-scope static-inline helpers carry the intent:
- CircularBuffer_TryClaim -> walk pool, claim first free slot
- CircularBuffer_Release -> walk pool, release matching slot
- CircularBuffer_SlotOwnsBase -> named predicate for the slot-walk match
_Destroy is now single-exit fall-through, retiring a pre-existing
MISRA 15.5 violation. cppcheck-misra count drops 88 -> 87.
The lock/unlock pair still wraps every slot walk; the error reporting
stays outside the critical section, matching the existing pattern.
Helpers are forward-declared at the top and defined immediately
beneath their first caller, matching SolidSyslogFileBlockDevice.c.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous shape held the LockConfig critical section around the whole slot walk, including the CircularBuffer_Initialise call. That becomes a problem the moment another E11 class has a non-trivial Initialise (FatFs file open, TLS context setup, ...) or the integrator maps LockConfig to taskENTER_CRITICAL on FreeRTOS — interrupts would be disabled for the full Initialise. TryClaim now locks per slot probe: lock, check, claim (mark InUse), unlock. Initialise runs once, outside any critical section. The slot is safe to initialise lock-free because the InUse=true flag has already reserved it against parallel Creates, and no other task can yet hold the handle (we haven't returned from _Create). Release / _Destroy uses the same per-iteration locking but Cleanup stays inside the critical section: releasing the lock between match and Cleanup would let a concurrent Destroy of the same handle race, and releasing it after marking InUse=false would let a concurrent Create grab the slot and race Initialise vs Cleanup on the same memory. The asymmetry is intentional and worth documenting for the rest of E11: Create can offload Initialise outside the lock; Destroy cannot offload Cleanup without a third "Pending" slot state. For CircularBuffer specifically Cleanup is 8 stores; on classes where Cleanup is expensive, integrators should map LockConfig to a sleeping mutex (pthread_mutex_t, FreeRTOS Semaphore) rather than a critical section. The existing CreateAcquiresAndReleasesConfigLock / DestroyAcquires... tests still pass: at default SOLIDSYSLOG_CIRCULAR_BUFFER_POOL_SIZE=1 "once per probed slot" equals "once total". A tunable-override preset with pool size > 1 would pin per-iteration locking observably; not adding one yet. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The TryClaim / Release / SlotOwnsBase static-inline split was making the file harder to read, not easier. Reverting the extraction so both public functions hold the whole walk inline; intent is to factor it differently from a clean starting point. Behaviour is unchanged: per-iteration LockConfig/UnlockConfig in both walks, Initialise runs outside any lock, Cleanup stays inside its per-iteration critical section, Fallback short-circuits Destroy, unknown handles report a WARNING. Tests still pass (1127 ran, 0 failures); cppcheck-misra count stays at 87, no new suppressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pool group: - New MakeBuffer() fixture method removes the (mutex, ring, sizeof(ring)) repetition at every Create call site. - New CHECK_IS_FALLBACK(buf, pool) macro turns the bare-CHECK trio in FillingPoolThenOverflowReturnsDistinctFallback into three named CHECK_TEXT calls that say which constituent failed and why. - Two new tests pin the per-iteration locking property added in 4a34777 -- they assert LockCallCount == SOLIDSYSLOG_CIRCULAR_BUFFER_POOL_SIZE for both Create-when-full and Destroy-of-last-issued. At default POOL_SIZE=1 these reduce to ONCE (the existing on-first-match tests already cover that); at POOL_SIZE > 1 they observe the per-iteration shape. - Existing CreateAcquiresAndReleasesConfigLock / Destroy... renamed to "...OnFirstFreeSlot" / "...OnFirstSlotMatch" to make their shape explicit alongside the new tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Eliminates claimedIndex from _Create. The slot-walk now tracks a struct SolidSyslogBuffer* initialised to &Fallback; a successful claim swings it to the slot's base pointer, and the loop breaks on the first non-Fallback observation. The single pointer carries both "have we claimed" and "which handle do we return". To make this work without a parallel index, CircularBuffer_Initialise gains the same base-pointer signature that _Cleanup already has -- both vtable lifecycle hooks now take struct SolidSyslogBuffer* and recover the concrete struct via CircularBuffer_SelfFromBase. The existing 11.3 line-pinned suppression follows the cast one line down inside the updated Initialise body; no new suppression sites. The comparisons against &Fallback are repeated -- next step is probably an IsFallback(p) helper to wrap them. Behaviour unchanged; 1127 tests pass, cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lifts the lock/check/claim/unlock sequence into a per-slot helper. TryClaimSlot returns the slot's base pointer when the slot was free and is now claimed, or &Fallback when the slot was already in use. _Create's loop body collapses to a single assignment and the break. The handle returned is exactly what _Create wants to either pass to Initialise or hand back to the caller, so no second walk or index recovery is needed. Sits immediately above _Create -- defined before first use, no forward declaration. 1127 tests pass; cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The "Try" in TryClaimSlot was misleading -- the function never fails, it always returns a usable handle, just sometimes the Fallback. "AcquireIfFree" reads honestly: it acquires the entry if free, otherwise hands back the Fallback. Drops the "Slot" terminology that was leaking the file-internal struct Slot name into the helper. The local "claimed" similarly read past-tense; the variable starts as &Fallback, which is the un-claimed state. "handle" matches the term the public API already uses for the buffer pointers it issues, and reads cleanly at both call sites: handle = &Fallback; (haven't found anything yet) handle = CircularBuffer_AcquireIfFree(i); 1127 tests pass; cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per CLAUDE.md "Function Ordering": _Create first, _Destroy second, helpers defined directly below their first caller with forward declarations grouped at the top. CircularBuffer_AcquireIfFree moves from above _Create to immediately below it, joined by a forward declaration alongside the Fallback_Read / Fallback_Write decls. No behaviour change; 1127 tests pass, cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The three "handle != &Fallback" / "base != &Fallback" sites in _Create and _Destroy now read as a named predicate. The Fallback sentinel address is now mentioned in exactly one place in the file -- the helper body -- so a future change of the sentinel mechanism (e.g. a per-class null-object table or a different signalling strategy) is a one-line edit. Skipping the symmetric HandleIsInvalid for now: no production site uses the negative form, and tests don't see &Fallback today. Easy follow-up when a test (or a future production guard-clause) wants positive-logic against "this is the fallback". 1127 tests pass; cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_Create now reads top-to-bottom as: handle = AcquireFirstFree(); if HandleIsValid -> Initialise else -> Error return handle; The pool walk lives one level down in AcquireFirstFree, where the loop bound and the per-iteration AcquireIfFree call are the only shape. _Create itself is six executable lines of pure intent. Function order: AcquireFirstFree is defined directly below _Create; AcquireIfFree directly below AcquireFirstFree (its first caller); HandleIsValid directly below that. _Destroy follows. Naming kept parallel with AcquireIfFree -- both are verb + free- condition with no "Handle" suffix; the `handle` variable at the call site documents the return shape. Trivial to bulk-rename both to *Handle later if the file gets more such helpers. 1127 tests pass; cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pool[poolIndex].InUse reads as a unit at every site; the name ties the index to the only collection it can index in the file, matching FileBlockDevice.c's blockIndex pattern. Applied to AcquireIfFree's parameter, AcquireFirstFree's loop, and _Destroy's loop -- all three are indices into Pool. 1127 tests pass; cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AcquireIfFree's guard now reads as a near-quote of the function name: "if PoolItemIsFree, claim it". The !InUse negation is hidden behind the predicate, leaving the function body in pure positive logic. Left _Destroy's "Pool[poolIndex].InUse && ..." check alone -- it wants the positive in-use sense, and PoolItemIsFree's inverse would be negative logic at the call site. If the symmetry matters later, PoolItemIsInUse is the natural pair to add. 1127 tests pass; cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The two lines that actually do the claim -- mark the pool item in-use and hand back its base pointer -- now live in a dedicated Acquire helper. The vocabulary nests: AcquireFirstFree -> walk pool, return first acquired AcquireIfFree -> probe one slot, lock-check-Acquire-unlock Acquire -> unconditional mark-and-return for one slot AcquireIfFree's body now reads as a sentence: if PoolItemIsFree, Acquire it. 1127 tests pass; cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_Destroy no longer treats &Fallback as a privileged input. The Fallback singleton is never installed in Pool, so the slot-walk simply doesn't match it and falls through to the existing WARNING path. The contract changes from Destroy(&Fallback) -> silent Destroy(other unknown handle) -> WARNING to a single uniform rule: Destroy(handle not currently issued by Create) -> WARNING regardless of whether that handle was once issued by Create (pool-exhaustion Fallback) or never was (stack-fabricated stranger, double-Destroy, etc). The function body drops one indent level and the asymmetric "we silently swallow the Fallback we issued you" special case. The existing DestroyOfUnknownHandleReportsWarning test already covers the WARNING path; the now-incorrect DestroyOfFallbackHandleIsSilent test is deleted (its assertion no longer holds). HandleIsValid stays -- still used by _Create and AcquireFirstFree. 1126 tests pass (-1 from the deletion); cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_Destroy now has two phases at the top level: 1. Find the matching poolIndex by scanning constant slot addresses (no lock needed -- &Pool[i].Object.Base never moves). 2. If a match was found, acquire the lock once, check InUse, conditionally Cleanup + free + release. Single lock acquisition per Destroy at most. Lock-count contract changes accordingly: - Pool-issued, currently-in-use handle: 1 lock (was 1..N). - Pool-issued, already-destroyed handle: 1 lock (was 1..N). - Unknown handle (no address match): 0 locks (was N). Externally-visible behaviour for the integrator is unchanged: live handles release cleanly, stale or stranger handles get the WARNING. The "stale-pool-address" case (double-Destroy or use-after-Destroy) already went down the WARNING path before -- the compound condition in the old loop just failed -- so this isn't a new failure mode, just made explicit. Tests updated: - DestroyAcquiresAndReleasesConfigLockOnFirstSlotMatch -> DestroyOfPooledHandleLocksOnce. The "OnFirstSlotMatch" qualifier is now obsolete; any pool-issued handle locks exactly once. - DestroyLocksOncePerSlotProbedUntilMatch (N locks) replaced by DestroyOfUnknownHandleDoesNotLock (NEVER). The N-locks property no longer holds and the zero-locks property is new and worth pinning. 1126 tests pass; cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
&Pool[poolIndex].Object.Base appeared at two unrelated sites -- Acquire's return and _Destroy's address-match scan -- with the same "convert pool index to the handle the integrator sees" meaning. Lifted into HandleFromIndex(poolIndex) so each site reads as intent, not field navigation. 1126 tests pass; cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pool[poolIndex].InUse mutations now go through named verbs -- MarkInUse(poolIndex) from Acquire, MarkFree(poolIndex) from _Destroy's release block. The field-write is hidden the same way PoolItemIsFree hides the field-read, so the file reads as a consistent vocabulary at one level of abstraction. Pair-of-helpers, not single-helper-with-bool-arg, so call sites don't read "set in-use to false" and instead read "mark free" -- intent at the verb, no boolean to translate. 1126 tests pass; cppcheck-misra stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_Destroy's head reduces to one line of intent:
poolIndex = IndexFromHandle(base);
The find loop now lives in its own function -- the natural inverse
of HandleFromIndex(i). Together the pair forms a closed conversion:
HandleFromIndex(i) -> base pointer for slot i
IndexFromHandle(base) -> slot index for that pointer, or
POOL_SIZE if base wasn't issued
_Destroy now reads at one level of abstraction: get the index,
release-if-in-use under lock, warn-if-not-released.
1126 tests pass; cppcheck-misra stays at 87.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The "poolIndex < SOLIDSYSLOG_CIRCULAR_BUFFER_POOL_SIZE" check was
shorthand for "IndexFromHandle returned a real index, not the
sentinel". Naming it makes the condition self-documenting:
if (CircularBuffer_PoolIndexIsValid(poolIndex))
Subject-IS-adjective form matches the other predicates in the file
(PoolItemIsFree, HandleIsValid).
1126 tests pass; cppcheck-misra stays at 87.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Raw Pool[poolIndex].InUse reads collapse to a single site. The
field's full surface in the file is now:
PoolItemIsInUse(poolIndex) -- the only read
MarkInUse(poolIndex) -- the only set-to-true
MarkFree(poolIndex) -- the only set-to-false
Everything else expresses intent. PoolItemIsFree becomes the
negation of PoolItemIsInUse, so a future representation change
(atomic flag, packed bitset, separate free-list, etc) touches just
the three accessor functions.
1126 tests pass; cppcheck-misra stays at 87.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Symmetric counterpart to AcquireIfFree: lock, check IsInUse, do
the work (Cleanup + MarkFree), unlock, return whether anything was
freed. _Destroy collapses to the lifecycle skeleton:
poolIndex = IndexFromHandle(base);
if PoolIndexIsValid -> released = FreeIfInUse(poolIndex);
if not released -> warn;
FreeIfInUse takes only the poolIndex, mirroring AcquireIfFree's
single-arg signature. The handle for Cleanup is recomputed
internally via HandleFromIndex -- no risk of a base argument
that diverges from the index.
1126 tests pass; cppcheck-misra stays at 87.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The loop now iterates poolIndex consistently with AcquireFirstFree;
the function's return becomes `result`, naming its role rather than
its type. The previous shape used the inconsistent `i` to avoid
shadowing -- this is the cleaner resolution.
Final state of CircularBufferStatic.c on this branch:
- Public API: _Create and _Destroy each read as 4-6 lines of pure
intent against named helpers.
- Helpers organised three levels deep:
* Composition -- AcquireFirstFree, AcquireIfFree, IndexFromHandle,
FreeIfInUse, Acquire
* Predicates -- HandleIsValid, PoolIndexIsValid,
PoolItemIsFree (-> chains through PoolItemIsInUse)
* Atomic ops -- MarkInUse, MarkFree, HandleFromIndex,
PoolItemIsInUse
- Pool[poolIndex].InUse touched in exactly three places:
one read (PoolItemIsInUse), one set-true (MarkInUse),
one set-false (MarkFree).
- &Fallback referenced in exactly two places: the static initialiser
and HandleIsValid.
- &Pool[poolIndex].Object.Base computed in exactly one place
(HandleFromIndex).
- Zero MISRA warnings in this TU; zero new cppcheck suppressions
or NOLINT comments anywhere on the branch since main.
- Lock duration: per-iteration around Acquire (so Initialise runs
unlocked); single acquire on _Destroy only when a pool address
matches.
- 1126 tests pass; cppcheck-misra count 87 (was 88 on main).
This is the reference shape for the rest of E11.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds DestroyOfStaleHandleReportsWarning. Exercises the FreeIfInUse path where IndexFromHandle DOES find a matching slot (so PoolIndexIsValid is true and the lock is taken) but PoolItemIsInUse is false because the slot was already released -- the double-Destroy / use-after-Destroy case introduced by the scan-then-release split. Brings CircularBufferStatic.c to 100% line, 100% function, and 100% branch coverage (68/68, 16/16, 18/18). The whole-codebase line + function coverage stays above the CI floor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tests now read as a 1-2 line setup followed by an act and a check,
with the noise lifted out into shared fixture members and macros.
- TEST_BASE CircularBufferFixture holds buffer, readData, readSize
and the Write(text) / Write(data, size) / Read() helpers. Groups
1, 2, 3 derive via TEST_GROUP_BASE; each declares only its own
ring storage and chooses its mutex source in setup/teardown.
Eliminates ~30 lines of duplicated declarations across the
three groups.
- CHECK_LAST_READ_RECORD(expected, size) macro at file scope
replaces the LONGS_EQUAL(size, readSize) + MEMCMP_EQUAL(...)
pair that appeared in 8+ tests. Wraps with NOLINTBEGIN/END so the
macro preserves __FILE__/__LINE__ at the test's call site.
- The Pool group teardown now guards against destroying nullptr
pool slots / nullptr overflow -- those previously emitted a
WARNING through the still-installed ErrorHandlerFake after the
body's assertions, which was harmless but noisy.
- Small cleanups:
OverflowingWriteIsDropped's local `overflow` -> `extra` (the
Pool group has an `overflow` field; the name clash was only
visual but it confused the reader).
WrapsAroundEndOfStorage drops the inline `enum {CYCLES,...}`
in favour of constexpr locals.
WriteExceedingMaxMessageSizeIsDropped drops the bespoke
destination buffer -- the fixture's readData is fine.
1127 tests pass; static.c coverage stays 100% line / 100% function /
100% branch; cppcheck-misra count stays at 87; clang-tidy, sanitize,
clang-format, plain cppcheck all green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captures the locked-in shape, the reference structure of *Static.c (this is the E11 reference for every other class), the test pinning strategy, the mid-flight course correction on helper extraction, the gate posture, and the open questions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR migrates SolidSyslogCircularBuffer from a caller-provided storage-blob model to a static-pool architecture using caller-allocated uint8_t ring buffers. The public API changes to SolidSyslogCircularBuffer_Create(mutex, ring, ringBytes); private lifecycle, static-pool Create/Destroy, tests, consumers, build options, and documentation are updated accordingly. ChangesCircular Buffer Pool Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 (✔️ 1133 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
![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: 3
🧹 Nitpick comments (2)
Core/Source/SolidSyslogCircularBufferStatic.c (1)
59-114: ⚡ Quick winPlace
_Destroyimmediately after_Createin this source file.Public
_Destroyis currently below helper definitions. Reorder so_Destroyis the second function, then keep helpers below their first caller.As per coding guidelines: "Within a source file, functions are ordered top-down: (1)
_Createfirst, (2)_Destroysecond, (3) other public functions in call order, (4) helper functions forward-declared at top ...".Also applies to: 116-128
🤖 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 `@Core/Source/SolidSyslogCircularBufferStatic.c` around lines 59 - 114, Move the public _Destroy function to immediately follow the public _Create function in this source file so _Create is first and _Destroy is second; then keep the other public functions (e.g., CircularBuffer_AcquireFirstFree, CircularBuffer_AcquireIfFree) and helper functions (e.g., CircularBuffer_PoolItemIsFree, CircularBuffer_PoolItemIsInUse, CircularBuffer_Acquire, CircularBuffer_MarkInUse, CircularBuffer_HandleFromIndex, CircularBuffer_HandleIsValid) below their first caller, adding forward declarations for any helper moved below its caller to preserve build order and visibility per the project ordering guideline.Tests/SolidSyslogCircularBufferTest.cpp (1)
13-13: 💤 Low valueRemove unused include
SolidSyslogPrival.h.The
#include "SolidSyslogPrival.h"at line 13 is not used anywhere in this test file. Remove it to keep the include list clean.🤖 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 `@Tests/SolidSyslogCircularBufferTest.cpp` at line 13, Remove the unused include by deleting the line that includes "SolidSyslogPrival.h" from Tests/SolidSyslogCircularBufferTest.cpp; ensure no references to symbols from SolidSyslogPrival remain in functions or tests (e.g., any use of prival-related identifiers) and run the test compile to confirm no missing dependencies after removing the include.
🤖 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 `@docs/misra-deviations.md`:
- Line 835: The phrase "caller-supplied ring memory in messages" is ambiguous
about units; update the text around the ring sizing macro (the sentence
containing "caller-supplied ring memory in messages") to explicitly state bytes
and show how to compute it from a maximum message count — e.g., reword to "size
caller-supplied ring memory in bytes from a max-message count" or similar, and
ensure any reference to the ring sizing macro mentions "bytes" (not just
"messages") so readers know the macro takes a byte size.
In `@misra_suppressions.txt`:
- Line 27: The suppression for misra-c2012-11.3 attributed to D.002 referring to
SolidSyslogCircularBuffer must be removed or rejustified: either delete the line
"misra-c2012-11.3:Core/Source/SolidSyslogCircularBuffer.c:91" from
misra_suppressions.txt and refactor CircularBuffer_SelfFromBase to avoid the
base-to-derived cast (replace the cast with a safe accessor or container/lookup
that returns a SolidSyslogCircularBuffer* without casting), or keep a
suppression but update its justification to a correct deviation entry (not
D.002) and add a matching explanation in docs/misra-deviations.md referencing
SolidSyslogCircularBuffer and the reason for the base-to-derived cast; ensure
the change references the function CircularBuffer_SelfFromBase and the
SolidSyslogCircularBuffer type so reviewers can locate the fix.
---
Nitpick comments:
In `@Core/Source/SolidSyslogCircularBufferStatic.c`:
- Around line 59-114: Move the public _Destroy function to immediately follow
the public _Create function in this source file so _Create is first and _Destroy
is second; then keep the other public functions (e.g.,
CircularBuffer_AcquireFirstFree, CircularBuffer_AcquireIfFree) and helper
functions (e.g., CircularBuffer_PoolItemIsFree, CircularBuffer_PoolItemIsInUse,
CircularBuffer_Acquire, CircularBuffer_MarkInUse,
CircularBuffer_HandleFromIndex, CircularBuffer_HandleIsValid) below their first
caller, adding forward declarations for any helper moved below its caller to
preserve build order and visibility per the project ordering guideline.
In `@Tests/SolidSyslogCircularBufferTest.cpp`:
- Line 13: Remove the unused include by deleting the line that includes
"SolidSyslogPrival.h" from Tests/SolidSyslogCircularBufferTest.cpp; ensure no
references to symbols from SolidSyslogPrival remain in functions or tests (e.g.,
any use of prival-related identifiers) and run the test compile to confirm no
missing dependencies after removing the include.
🪄 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: 368a0d05-9ea9-4e10-a795-654bd598867f
📒 Files selected for processing (21)
Bdd/Targets/FreeRtos/main.cBdd/Targets/Windows/BddTargetWindows.cCLAUDE.mdCMakeLists.txtCore/Interface/SolidSyslogCircularBuffer.hCore/Interface/SolidSyslogTunablesDefaults.hCore/Source/CMakeLists.txtCore/Source/SolidSyslogCircularBuffer.cCore/Source/SolidSyslogCircularBufferPrivate.hCore/Source/SolidSyslogCircularBufferStatic.cCore/Source/SolidSyslogErrorMessages.hDEVLOG.mdREADME.mdSKILL.mdTests/SolidSyslogBlockStoreDrainOrderingTest.cppTests/SolidSyslogCircularBufferTest.cppTests/SolidSyslogTest.cppdocs/NAMING.mddocs/iec62443.mddocs/misra-deviations.mdmisra_suppressions.txt
Comment on lines
+116
to
+127
| void SolidSyslogCircularBuffer_Destroy(struct SolidSyslogBuffer* base) | ||
| { | ||
| size_t poolIndex = CircularBuffer_IndexFromHandle(base); | ||
| bool released = false; | ||
| if (CircularBuffer_PoolIndexIsValid(poolIndex)) | ||
| { | ||
| released = CircularBuffer_FreeIfInUse(poolIndex); | ||
| } | ||
| if (!released) | ||
| { | ||
| SolidSyslog_Error(SOLIDSYSLOG_SEVERITY_WARNING, SOLIDSYSLOG_ERROR_MSG_CIRCULARBUFFER_UNKNOWN_DESTROY); | ||
| } |
Contributor
There was a problem hiding this comment.
Destroy warns on fallback handles returned by Create.
Line 61 allows Create to return &Fallback, but Lines 120–127 can never mark that handle as released, so Destroy emits SOLIDSYSLOG_ERROR_MSG_CIRCULARBUFFER_UNKNOWN_DESTROY for a valid Create→Destroy flow under pool exhaustion.
Proposed fix
void SolidSyslogCircularBuffer_Destroy(struct SolidSyslogBuffer* base)
{
size_t poolIndex = CircularBuffer_IndexFromHandle(base);
bool released = false;
- if (CircularBuffer_PoolIndexIsValid(poolIndex))
+ if (base == &Fallback)
+ {
+ released = true;
+ }
+ else if (CircularBuffer_PoolIndexIsValid(poolIndex))
{
released = CircularBuffer_FreeIfInUse(poolIndex);
}
if (!released)
{
SolidSyslog_Error(SOLIDSYSLOG_SEVERITY_WARNING, SOLIDSYSLOG_ERROR_MSG_CIRCULARBUFFER_UNKNOWN_DESTROY);
}
}
Owner
Author
There was a problem hiding this comment.
This is the designed behaviour, not a bug — declining the suggestion.
Earlier in this branch (commit 6500440, "feat: S11.01 drop Fallback special-case from _Destroy") we deliberately removed the if (base == &Fallback) return; short-circuit from _Destroy and unified the contract to:
any handle that is not currently issued by
_CreatereportsSOLIDSYSLOG_SEVERITY_WARNING— regardless of whether it was once issued (pool-exhaustion Fallback, double-Destroy) or never was (stack-fabricated stranger).
The rationale, captured in that commit message and in the DEVLOG entry:
- Simpler contract for the integrator: one rule covers all "didn't come from a live
_Create" cases. The Fallback exception was load-bearing only for the silent-on-Fallback property — which is itself questionable: a Fallback handle means the system hit pool exhaustion at some point, and the integrator destroying it is a useful place to surface that information one more time. _Destroyreads at one level of abstraction without the special case — that one-indent-flatter shape is the reference for every other E11 class.- The Fallback is a no-op handle that never accepts Writes or returns data on Reads, so the WARNING does not signal any data loss at Destroy time — it only flags the prior exhaustion.
The error handler is null-object by default, so an integrator who has not installed a handler sees nothing at all; one who has, sees the same uniform message for any unknown handle. Pinned in tests DestroyOfUnknownHandleReportsWarning and DestroyOfStaleHandleReportsWarning.
Contributor
There was a problem hiding this comment.
@DavidCozens, understood — withdrawing the suggestion. The unified "any handle not currently issued by _Create emits WARNING" contract is well-reasoned: it removes a Fallback special-case that would have diverged from the reference E11 shape, surfaces the prior pool-exhaustion event at destroy time without implying data loss, and keeps the null-object default handler harmless for integrators who haven't installed one. The test coverage in DestroyOfUnknownHandleReportsWarning and DestroyOfStaleHandleReportsWarning makes the contract explicit.
(´• ω •`) nods and hops away
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 394
File: Core/Source/SolidSyslogCircularBufferStatic.c:116-127
Timestamp: 2026-05-18T05:42:33.698Z
Learning: In DavidCozens/solid-syslog, `SolidSyslogCircularBuffer_Destroy` (Core/Source/SolidSyslogCircularBufferStatic.c) intentionally emits `SOLIDSYSLOG_SEVERITY_WARNING` / `SOLIDSYSLOG_ERROR_MSG_CIRCULARBUFFER_UNKNOWN_DESTROY` for ALL handles not currently issued by `_Create`, including pool-exhaustion Fallback handles. This is the designed E11 reference contract: one uniform rule covers Fallback handles, double-Destroy, and stranger handles — no Fallback short-circuit. Do not flag this as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 283
File: Platform/OpenSsl/Source/SolidSyslogTlsStream.c:363-363
Timestamp: 2026-05-07T08:22:59.856Z
Learning: In this solid-syslog codebase, follow the documented contract: do not add NULL/defensive checks inside library/internal code paths (e.g., `_Create` functions) when the NULL case cannot occur under normal internal usage and is covered by framework or internal wiring guarantees. Only add validation at true system boundaries such as user input and external APIs. Where setup-time wiring contracts between the integrator and the library require specific function-pointer fields (e.g., `SolidSyslogTlsStreamConfig.sleep`), assume those required fields are present and intentionally omit NULL guards in internal creation functions.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 291
File: Example/FreeRtos/HelloWorld/CmsdkUart.c:17-42
Timestamp: 2026-05-08T17:19:38.382Z
Learning: In this codebase (DavidCozens/solid-syslog), internal driver/config/access structs may intentionally store raw pointers without NULL checks, `isInitialized` flags, or defensive struct copies. Treat this as an explicit project policy: do not flag store-by-pointer usage or missing NULL/initialization guards for these internal config/access structs when the pointer is provided by production callers and is guaranteed to point to `static const` objects with program lifetime (per CLAUDE.md). Only flag NULL/initialization issues when the pointer originates from potentially invalid/external/untrusted sources or when the code does not establish lifetime/guarantees.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 303
File: Example/Common/ExampleInteractive.h:14-17
Timestamp: 2026-05-09T15:56:13.853Z
Learning: During code review, ensure solid-syslog Tier 1/2 library callbacks in Core/ and Platform/ follow the CLAUDE.md “Callback Conventions”: the callback API must take a `void*` context parameter and the implementation must use the paired configuration field associated with that callback context. Do not apply this requirement to Tier 3 example handlers (e.g., Example/Common/, Example/FreeRtos/, such as `ExampleInteractiveSwitchHandler`/`ExampleInteractiveSetHandler`), which are allowed to use file-static globals per the documented example-tier pattern. If a multi-instance use-case arises, migrate Switch and Set together in a single cohesive change rather than piecemeal updates.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 372
File: Platform/FreeRtos/Source/SolidSyslogFreeRtosDatagram.c:0-0
Timestamp: 2026-05-15T13:09:11.811Z
Learning: When reviewing PRs that bulk-rename identifiers (especially “static function rename” changes) in this repo, check for unintended renames inside FreeRTOS macro usages. A too-broad regex can incorrectly prefix macro tokens used in `static const` initializers (e.g., `pdMS_TO_TICKS` from FreeRTOS). Look for signs like a renamed macro name being incorrectly prefixed (e.g., `FreeRtosDatagram_pdMS_TO_TICKS`), and if present, treat it as evidence the identifier-extraction regex was overly broad and requires correction/revert of the affected rename changes.
| misra-c2012-11.3:Core/Source/SolidSyslogBlockStore.c:165 | ||
| misra-c2012-11.3:Core/Source/SolidSyslogCircularBuffer.c:72 | ||
| misra-c2012-11.3:Core/Source/SolidSyslogCircularBuffer.c:89 | ||
| misra-c2012-11.3:Core/Source/SolidSyslogCircularBuffer.c:91 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate CircularBuffer implementation =="
fd -a 'SolidSyslogCircularBuffer.c$'
echo
echo "== Show the suppressed line and nearby context (expect line 91 to contain the cast/findings site) =="
nl -ba Core/Source/SolidSyslogCircularBuffer.c | sed -n '78,110p'
echo
echo "== Show cast helper patterns in CircularBuffer implementation =="
rg -n -C2 --type=c 'SelfFromBase|SelfFromStorage|\(struct SolidSyslogCircularBuffer\*\)' Core/Source/SolidSyslogCircularBuffer.c
echo
echo "== Show D.002 text for scope comparison =="
nl -ba docs/misra-deviations.md | sed -n '140,205p'Repository: DavidCozens/solid-syslog
Length of output: 333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Show the suppressed line and nearby context (lines 78-110) =="
cat -n Core/Source/SolidSyslogCircularBuffer.c | sed -n '78,110p'
echo
echo "== Show cast patterns in CircularBuffer (SelfFromBase|SelfFromStorage|cast to struct SolidSyslogCircularBuffer*) =="
rg -n -C2 --type=c 'SelfFromBase|SelfFromStorage|\(struct SolidSyslogCircularBuffer\*\)' Core/Source/SolidSyslogCircularBuffer.c
echo
echo "== Show D.002 deviation section from misra-deviations.md =="
cat -n docs/misra-deviations.md | sed -n '140,205p'
echo
echo "== Verify the exact suppression entry in misra_suppressions.txt at line 27 =="
cat -n misra_suppressions.txt | sed -n '20,35p'Repository: DavidCozens/solid-syslog
Length of output: 7428
Remove this suppression or realign it to a non-D.002 justification.
docs/misra-deviations.md D.002 explicitly excludes SolidSyslogCircularBuffer from its scope (lines 168–173: "moved off this pattern under E11 (S11.01)... the instance struct now lives in a library-internal static pool"). However, the Rule 11.3 suppression at line 91 remains attributed to D.002. The cast (struct SolidSyslogCircularBuffer*) base in CircularBuffer_SelfFromBase is a base-to-derived pointer cast that is no longer covered by the deviation. Either remove the suppression and refactor the code to eliminate the cast, or document a separate justification if the cast is intentionally retained.
🤖 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 `@misra_suppressions.txt` at line 27, The suppression for misra-c2012-11.3
attributed to D.002 referring to SolidSyslogCircularBuffer must be removed or
rejustified: either delete the line
"misra-c2012-11.3:Core/Source/SolidSyslogCircularBuffer.c:91" from
misra_suppressions.txt and refactor CircularBuffer_SelfFromBase to avoid the
base-to-derived cast (replace the cast with a safe accessor or container/lookup
that returns a SolidSyslogCircularBuffer* without casting), or keep a
suppression but update its justification to a correct deviation entry (not
D.002) and add a matching explanation in docs/misra-deviations.md referencing
SolidSyslogCircularBuffer and the reason for the base-to-derived cast; ensure
the change references the function CircularBuffer_SelfFromBase and the
SolidSyslogCircularBuffer type so reviewers can locate the fix.
Owner
Author
There was a problem hiding this comment.
Accepted as a documentation update in 9bd1d39 — the suppression line itself stays put.
The cast inside CircularBuffer_SelfFromBase is the vtable downcast (struct SolidSyslogBuffer* → struct SolidSyslogCircularBuffer*), not the caller-storage void* cast that E11 retired. That shape is shared by every other class listed in this D.002 entry — they all have an equivalent SelfFromBase cast for the same reason — so the deviation continues to cover it. The corresponding paragraph in docs/misra-deviations.md now says so explicitly, distinguishing the vtable-downcast concern from the (now-gone) caller-storage concern for CircularBuffer.
Removing the cast would mean abandoning the vtable interface (SolidSyslogBuffer_Read / _Write dispatch through a struct SolidSyslogBuffer*), which would ripple through every consumer — not a S11.01 change. If the deviation should be split into two separate blocks (caller-storage vs vtable-downcast) for the whole library, that's a follow-up worth doing across the codebase rather than just this entry.
Contributor
There was a problem hiding this comment.
@DavidCozens — understood, and the clarification is well-reasoned. The vtable downcast in CircularBuffer_SelfFromBase is structurally identical to the SelfFromBase pattern in every other class under D.002, so the deviation continues to cover it. My original comment incorrectly conflated the two distinct cast concerns: the (now-retired) caller-storage void* cast and the (still-present) vtable downcast. The documentation update in 9bd1d39 making that distinction explicit is exactly the right fix. Agreed that splitting D.002 into two blocks (caller-storage vs. vtable-downcast) for the whole library is a sensible follow-up rather than something to do here.
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 394
File: misra_suppressions.txt:27-27
Timestamp: 2026-05-18T05:42:55.403Z
Learning: In the solid-syslog codebase (`misra_suppressions.txt` and `docs/misra-deviations.md`), D.002 (Rules 11.2/11.3/11.5) covers two distinct cast patterns: (1) the vtable downcast (`struct SolidSyslogBuffer*` → `struct SolidSyslogCircularBuffer*` via `CircularBuffer_SelfFromBase`, and analogous `SelfFromBase` helpers in every other class) which is retained for all classes including those migrated under E11; and (2) the caller-storage `void*` cast which E11-migrated classes (currently only `SolidSyslogCircularBuffer`) no longer use. Do not flag the `SelfFromBase` vtable downcast as uncovered by D.002 — it is explicitly documented in `docs/misra-deviations.md` (updated in commit 9bd1d39 of PR `#394`). A potential future refactor to split D.002 into two separate blocks for the whole library was noted as out of scope for S11.01.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Addresses the analyze-iwyu CI failure and the two actionable CodeRabbit comments worth acting on: - Tests/SolidSyslogCircularBufferTest.cpp: explicit `#include <stdint.h>` for uint8_t. The new TEST_GROUP_BASE-derived groups declare `uint8_t ring[...]` directly; uint8_t previously came in transitively through a SolidSyslog header. (IWYU finding.) - scripts/iwyu_filter.py: extend the filter to drop CppUTest macro-generated `class TEST_<Group>_<Test>_Test;` forward-decl additions. Same category of false positive as the existing `- #include "CppUTest/TestHarness.h"` filter -- IWYU doesn't model CppUTest's TEST() / TEST_GROUP_BASE() macro expansion. Filter smoke-tested locally; running IWYU in the clang container reports every TU "has correct #includes/fwd-decls" with no actionable findings. - docs/misra-deviations.md (RING_BYTES section): reword "caller-supplied ring memory in messages" -> "caller-supplied ring buffer in bytes, derived from a maximum message count" with a worked example. The macro returns bytes; the old wording was unit-ambiguous. (CodeRabbit minor.) - docs/misra-deviations.md (D.002 section): extend the E11-exception paragraph to explicitly note that the surviving 11.3 suppression for Core/Source/SolidSyslogCircularBuffer.c covers the vtable downcast inside CircularBuffer_SelfFromBase, not the caller-storage void* cast that's now gone. Same deviation block continues to cover both shapes because both are 11.3 firings driven by the same "interface decoupled from concrete type" design. (CodeRabbit major -- accepted as a documentation/comment update rather than removal; the cast itself is the standard OO-in-C vtable downcast that every other class in the deviation already shares.) The three other CodeRabbit findings (Destroy-warns-on-Fallback, function-order, "unused" SolidSyslogPrival.h) are addressed by reply on the PR rather than code change -- see PR #394 thread. 1127 tests pass; IWYU clean; cppcheck-misra count stays at 87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Going through the two nits in the review summary, both declined with rationale:
The convention in this repo is "helpers defined immediately beneath the function that first calls them", established in
It is used — provides Three test bodies ( |
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1133 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/iwyu_filter.py (1)
128-135: ⚡ Quick winUse descriptive variable names in list comprehensions.
The variable name
lis ambiguous and can be confused with1orI. Use a more descriptive name likelineoraddition.📝 Proposed fix for improved readability
- add_real = [ - l for l in add_lines - if l.strip() and not _is_filtered_addition(l) - ] - remove_real = [ - l for l in remove_lines - if l.strip() and not _is_filtered_removal(l) - ] + add_real = [ + line for line in add_lines + if line.strip() and not _is_filtered_addition(line) + ] + remove_real = [ + line for line in remove_lines + if line.strip() and not _is_filtered_removal(line) + ]🤖 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 `@scripts/iwyu_filter.py` around lines 128 - 135, Rename ambiguous comprehension iteration variables from `l` to descriptive names: use `line` (or `addition`/`removal`) in the comprehensions that build `add_real` and `remove_real`. Update the two list comprehensions that iterate over `add_lines` and `remove_lines` and call `_is_filtered_addition` / `_is_filtered_removal` so they use `line` (e.g., `for line in add_lines`) instead of `l` to improve readability.
🤖 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.
Nitpick comments:
In `@scripts/iwyu_filter.py`:
- Around line 128-135: Rename ambiguous comprehension iteration variables from
`l` to descriptive names: use `line` (or `addition`/`removal`) in the
comprehensions that build `add_real` and `remove_real`. Update the two list
comprehensions that iterate over `add_lines` and `remove_lines` and call
`_is_filtered_addition` / `_is_filtered_removal` so they use `line` (e.g., `for
line in add_lines`) instead of `l` to improve readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b054c7ca-c96e-4eab-af4c-e064618e567b
📒 Files selected for processing (3)
Tests/SolidSyslogCircularBufferTest.cppdocs/misra-deviations.mdscripts/iwyu_filter.py
✅ Files skipped from review due to trivial changes (1)
- docs/misra-deviations.md
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/SolidSyslogCircularBufferTest.cpp
Owner
Author
|
One more validation step done locally: full unit test suite re-run with Check count moves 2477 → 2481 (+4) — that's That clears the last bullet from my work list for S11.01. Ready when you are. |
This was referenced May 18, 2026
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 #392. First story of E11 (#29) — migrates
SolidSyslogCircularBufferaway from caller-supplied instance storage and onto a library-owned static pool. The resulting*Static.cis the reference shape for every other E11 class (BlockStore, FileBlockDevice, the platform Mutex / Stream / Datagram / File / Atomic / TLS / FatFs adapters); getting it right here saves churn later. PR #393 (already merged) landed theSolidSyslog_SetConfigLocksynchronisation seam this PR consumes.Change Description
Public API change
SolidSyslogCircularBuffer_Create(struct SolidSyslogMutex* mutex, uint8_t* ring, size_t ringBytes)returnsstruct SolidSyslogBuffer*. The instance bookkeeping struct is now library-pooled; the integrator still supplies the ring memory._Destroy(struct SolidSyslogBuffer* base)takes the handle the integrator was given.Three-TU split per E11 class
Core/Source/SolidSyslogCircularBuffer.c— vtable + ring logic + privateInitialise/Cleanup.Core/Source/SolidSyslogCircularBufferPrivate.h— struct + private signatures (TU-internal; tests do not include).Core/Source/SolidSyslogCircularBufferStatic.c— pool, public_Create/_Destroy, Fallback singleton, slot-walk synchronisation.Locked-in shape (the bits other E11 classes copy)
SOLIDSYSLOG_CIRCULAR_BUFFER_POOL_SIZE(default 1U),#ifndef-guarded, floor#error.SolidSyslog_LockConfig()wraps every slot probe. Per-iteration locking, not lock-once-around-loop.Initialiseruns outside the lock;Cleanupruns inside its per-iteration critical section. The asymmetry is intentional — releasing the lock aroundCleanupwould let a concurrentCreategrab the slot and raceInitialisevsCleanupon the same memory.struct SolidSyslogBufferwith a no-op vtable. Pool exhaustion returns&Fallback(Log/Service become no-ops; never NULL deref). One ERROR severity report on the transition._Destroycontract: any handle not currently issued by Create (unknown stranger / pool-issued but already destroyed / Fallback) reportsSOLIDSYSLOG_SEVERITY_WARNING. Same message; no per-case carve-outs.Initialise/Cleanup— both takestruct SolidSyslogBuffer* baseand recover the concrete struct viaCircularBuffer_SelfFromBase. The 11.3 cast lives in one TU (already deviated).*Static.cnever casts._Destroy: address-match walk is lock-free (pool addresses are file-scope statics that never move); the lock only wraps theInUsecheck +Cleanup+MarkFree. Lock count drops to 0 on unknown handles, 1 on matched handles, regardless of pool size.Reference structure of
*Static.c_Createand_Destroyare four lines of pure intent each. The helper tree splits at clean boundaries:AcquireFirstFree,AcquireIfFree,IndexFromHandle,FreeIfInUse,AcquirePoolItemIsFreechains throughPoolItemIsInUse;HandleIsValid;PoolIndexIsValidMarkInUse,MarkFree,HandleFromIndex,PoolItemIsInUseThe
.InUsefield is touched in three places total: one read (PoolItemIsInUse), one set-true (MarkInUse), one set-false (MarkFree).&Fallbackis referenced in two places (static initialiser +HandleIsValid).&Pool[poolIndex].Object.Baseis computed in exactly one place (HandleFromIndex).MISRA / suppression posture
cppcheck-suppress/NOLINTlines added on this branch — the E11 invariant.CircularBuffer.c'sSelfFromBasecast shifted two lines asInitialiseandCleanupsignatures changed; both shifts updated the existing entry, never added a new one.cppcheck-misracount: 87 (down 1 frommain's 88, gained by_Destroybecoming single-exit and then by the scan-then-release split).Tests
SolidSyslogCircularBufferPoolpins the pool-exhaustion Fallback contract, the ERROR report on exhaustion, lock counts on Create (per-iteration) and Destroy (zero on unknown, one on matched), and WARNING-on-unknown-or-stale.SolidSyslogCircularBufferandSolidSyslogCircularBufferMutextest groups restructured onto a sharedCircularBufferFixtureTEST_BASE withWrite(...)/Read()helpers and aCHECK_LAST_READ_RECORDmacro. Test bodies now read as setup → act → check with noSolidSyslogBuffer_*(buffer, ..., sizeof(...), &readSize)boilerplate.Test Evidence
All gates green on the host containers:
cmake --preset debug+SolidSyslogTestscmake --preset sanitize(ASan + UBSan) + testscmake --preset clang-debug(Clang container)cmake --preset tidy(clang-tidyerrors-as-warnings)cmake --preset cppcheckcmake --preset coverageclang-format --dry-run --Werrorcppcheck-misra(warning-mode)SolidSyslogCircularBufferStatic.cspecifically: 100% line (68/68), 100% function (16/16), 100% branch (18/18).Windows / BDD / FreeRTOS / TLS-integration jobs run in CI.
Areas Affected
_Create/_DestroyAPI change (caller-supplied instance storage gone; ring still caller-supplied).SOLIDSYSLOG_CIRCULAR_BUFFER_POOL_SIZEtunable.InitialiseandCleanupnow takebase.Downstream callers in this repo (
Bdd/Targets/,Tests/Bdd/Targets) were updated to the new_Createsignature in earlier commits on the branch.🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
New Features
Documentation