feat: S11.04 Core singleton stateful classes onto PoolAllocator#400
Merged
Conversation
Shared GoF null Structured Data for S11.04's sweep. Three classes (MetaSd, TimeQualitySd, OriginSd) need the same no-op fallback when their pool is exhausted; one Null type avoids three class-private copies. Get-only API — the instance is immutable and shared across consumers, so no _Create/_Destroy lifecycle is appropriate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Shared GoF null Sender for S11.04's sweep. UdpSender and SwitchingSender both need the same fallback when their pool is exhausted, and S11.05's StreamSender migration will reuse it too — one Null type keeps the fallback definition in one place. Send returns false (delivery semantics: "didn't send" — Service then takes the store-and-forward path or drops). Disconnect is a no-op. Get-only API because the instance is immutable and shared. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A Null Object is immutable and inherently shared — Create/Destroy implied a lifecycle that doesn't exist, and a Destroy call from one consumer would have torn down the vtable for any other consumer holding the same instance. Replace with a single Get() returning the shared static-initialised instance. Drops the SolidSyslogNullStore wrapper struct (no extra fields) in favour of returning &instance directly typed as SolidSyslogStore. All callers (test fixtures, BDD targets) updated. DestroyStore in the three BDD targets now no-ops when the store is the Null one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same reasoning as the NullStore flip: a Null Object is immutable and inherently shared, so Create/Destroy implied a lifecycle that didn't exist. The instance was already a function-scope static — lift to file scope, rename _Create to _Get, drop the no-op _Destroy. BlockStore's resolver and both test callers updated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per the E11 canonical 3-TU split. Internal struct moves to SolidSyslogPassthroughBufferPrivate.h; vtable + Initialise/Cleanup stay in SolidSyslogPassthroughBuffer.c; pool, public Create/Destroy, IndexFromHandle, CleanupAtIndex, and the class-private Fallback live in SolidSyslogPassthroughBufferStatic.c. The Buffer abstract base has no shared GoF null in this sweep (CircularBuffer is the only other consumer and uses its own private Fallback too), so PassthroughBuffer keeps a class-private fallback. Public API: _Destroy(void) → _Destroy(struct SolidSyslogBuffer*). Caller updates in PassthroughBufferTest.cpp and SolidSyslogTest.cpp. Pool size tunable: SOLIDSYSLOG_PASSTHROUGH_BUFFER_POOL_SIZE, default 1. Error messages: POOL_EXHAUSTED (ERROR severity) and UNKNOWN_DESTROY (WARNING severity). Pool tests: two new tests in SolidSyslogPassthroughBufferTest.cpp prove that filling to capacity returns distinct handles and the next Create returns a distinct no-op fallback. Generic per-probe lock and stale- handle behaviour is covered by SolidSyslogPoolAllocatorTest.cpp. Validated at SOLIDSYSLOG_PASSTHROUGH_BUFFER_POOL_SIZE=3 via the user- tunables override — full suite green. 100% line coverage on the two new TUs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per the E11 canonical 3-TU split. Internal struct moves to SolidSyslogUdpSenderPrivate.h; vtable, connect/disconnect logic, and the Send NULL-buffer guard stay in SolidSyslogUdpSender.c; pool, public Create/Destroy, IndexFromHandle, CleanupAtIndex, and the bad-config validation move to SolidSyslogUdpSenderStatic.c. The class-private NilUdpSender is gone — bad config and pool exhaustion both return the shared SolidSyslogNullSender_Get(). Public API: _Destroy(void) → _Destroy(struct SolidSyslogSender*). Caller updates in SolidSyslogUdpSenderTest.cpp, the BDD ServiceThread test, and all three BDD targets (Linux, Windows, FreeRTOS). The BDD targets now keep udpSender as a file-scope static so DestroySender() can pass it through. NullSender.Send flipped from false to true: a misconfigured Sender must drop messages at the null-object boundary, not retain them in the Store. The old NilUdpSender did this; the shared NullSender now inherits that behaviour. NullSenderTest assertion follows. The Send NULL-buffer SolidSyslog_Error call stays in UdpSender.c (not Static.c) — it's a runtime contract guard, not a pool/config error. The "ClassStatic.c is the only TU that calls Error" rule applies to pool/config errors; vtable-method runtime guards stay where the check is. Pool size tunable: SOLIDSYSLOG_UDP_SENDER_POOL_SIZE, default 1. Error messages: POOL_EXHAUSTED (ERROR severity) and UNKNOWN_DESTROY (WARNING severity). One new pool test in SolidSyslogUdpSenderTest.cpp proves capacity + overflow returns a distinct fallback. Validated at SOLIDSYSLOG_UDP_SENDER_POOL_SIZE=3. 100% line coverage on UdpSender.c, UdpSenderStatic.c, NullSender.c. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per the E11 canonical 3-TU split. SwitchingSenderPrivate.h holds the struct + Initialise/Cleanup decls; SwitchingSender.c keeps the vtable methods and selector-dispatch logic; SwitchingSenderStatic.c owns the pool, Create/Destroy, IndexFromHandle, CleanupAtIndex. The class-private NIL_SENDER is gone: pool exhaustion AND out-of-range selector both resolve to the shared SolidSyslogNullSender_Get(). Public API: _Destroy(void) → _Destroy(struct SolidSyslogSender*). Caller updates in SolidSyslogSwitchingSenderTest.cpp and all three BDD targets. BDD targets gain a file-scope `switchingSender` pointer. Two ZeroSenderCount / SelectorBeyondEnd tests flip from "Send returns false" to "Send returns true" — they now resolve to the shared NullSender, which drops on the floor. Same semantic as UdpSender's bad-setup path, by design. The test helper CreateSwitchingSender(count) gains a defensive Destroy of any previously-allocated sender. With singleton semantics the second call simply overwrote the slot; with pooling the second call would exhaust the pool and return the Fallback, leaving the first slot orphaned. The override-tests (Selector at last valid index, zero count) hit this path. Pool size tunable: SOLIDSYSLOG_SWITCHING_SENDER_POOL_SIZE, default 1. Error messages: POOL_EXHAUSTED (ERROR) and UNKNOWN_DESTROY (WARNING). One new pool test in SolidSyslogSwitchingSenderTest.cpp proves capacity + overflow returns a distinct fallback. Validated at SOLIDSYSLOG_SWITCHING_SENDER_POOL_SIZE=3. 100% line coverage on SwitchingSender.c and SwitchingSenderStatic.c. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per the E11 canonical 3-TU split. MetaSdPrivate.h holds the struct + Initialise/Cleanup decls; MetaSd.c keeps the vtable Format method and the per-field emit helpers; MetaSdStatic.c owns the pool, public Create/Destroy, IndexFromHandle, CleanupAtIndex, and the bad-config validation. The class-private nilMetaSd is gone — bad config and pool exhaustion both return SolidSyslogNullSd_Get(). Public API: _Destroy(void) → _Destroy(struct SolidSyslogStructuredData*). Caller updates in SolidSyslogMetaSdTest.cpp, SolidSyslogTest.cpp, and all three BDD targets. Pool size tunable: SOLIDSYSLOG_META_SD_POOL_SIZE, default 1. Error messages: POOL_EXHAUSTED (ERROR) and UNKNOWN_DESTROY (WARNING). One new pool test proves capacity + overflow returns a distinct fallback. Validated at SOLIDSYSLOG_META_SD_POOL_SIZE=3. 100% line coverage on MetaSd.c and MetaSdStatic.c. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per the E11 canonical 3-TU split. TimeQualitySd has no bad-config validation today (no NULL check on the getTimeQuality callback) and this refactor preserves that — validation belongs in a separate feature commit, not here. Pool exhaustion resolves to SolidSyslogNullSd_Get(). Public API: _Destroy(void) → _Destroy(struct SolidSyslogStructuredData*). Caller updates in TimeQualitySdTest.cpp, SolidSyslogTest.cpp, and all three BDD targets. Pool size tunable: SOLIDSYSLOG_TIME_QUALITY_SD_POOL_SIZE, default 1. Error messages: POOL_EXHAUSTED (ERROR) and UNKNOWN_DESTROY (WARNING). One new pool test proves capacity + overflow returns a distinct fallback. Validated at SOLIDSYSLOG_TIME_QUALITY_SD_POOL_SIZE=3. 100% line coverage on TimeQualitySd.c and TimeQualitySdStatic.c. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per the E11 canonical 3-TU split. OriginSdPrivate.h holds the struct, the size enums (ORIGIN_SOFTWARE_MAX, etc.), and Initialise/Cleanup decls; OriginSd.c keeps the vtable Format method, the pre-format static-prefix step, and the per-field emit helpers; OriginSdStatic.c owns the pool, Create/Destroy, IndexFromHandle, CleanupAtIndex. OriginSd_PreFormatStaticPrefix becomes a per-instance function taking the slot's `self` so each pool slot writes into its own FormattedStorage buffer (the largest per-slot footprint in the sweep — software + swVersion + enterpriseId pre-formatted at Create time). Public API: _Destroy(void) → _Destroy(struct SolidSyslogStructuredData*). Caller updates in OriginSdTest.cpp and all three BDD targets. Pool size tunable: SOLIDSYSLOG_ORIGIN_SD_POOL_SIZE, default 1. Error messages: POOL_EXHAUSTED (ERROR) and UNKNOWN_DESTROY (WARNING). One new pool test proves capacity + overflow returns a distinct fallback. Validated at SOLIDSYSLOG_ORIGIN_SD_POOL_SIZE=3. 100% line coverage on OriginSd.c and OriginSdStatic.c. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The migrated _Destroy signatures had the parameter named after the concrete class (sender, sd, buffer). NAMING.md is explicit: when the declared parameter type is the abstract base struct, the parameter name is "base". When it's the concrete class, it's "self". All these _Destroy functions take the abstract base type, so the parameter name is "base" across the board. Two test fixture helpers (PassthroughBuffer MakeBuffer, TimeQualitySd MakeSd) also realigned to match the CircularBuffer reference shape so clang-tidy's readability-make-member-function-const / readability-convert-member-functions-to-static rules don't trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 (13)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughMigrates several singleton stateful core components to fixed-size pool-backed implementations, introduces null-object ChangesCore Pool Allocator Migration
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1154 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: 4
🧹 Nitpick comments (2)
Core/Source/SolidSyslogUdpSenderStatic.c (1)
19-21: ⚡ Quick winRename file-scope statics to class-prefixed identifiers.
InUse,Pool, andAllocatorshould follow the file-scope static naming rule (class-prefixed function-style or screaming-snake style).As per coding guidelines `**/*.{c,h}`: file-scope statics `Class_Function` or `CLASS_SCREAMING_SNAKE`.Proposed rename
-static bool InUse[SOLIDSYSLOG_UDP_SENDER_POOL_SIZE]; -static struct SolidSyslogUdpSender Pool[SOLIDSYSLOG_UDP_SENDER_POOL_SIZE]; -static struct SolidSyslogPoolAllocator Allocator = {InUse, SOLIDSYSLOG_UDP_SENDER_POOL_SIZE}; +static bool UdpSender_InUse[SOLIDSYSLOG_UDP_SENDER_POOL_SIZE]; +static struct SolidSyslogUdpSender UdpSender_Pool[SOLIDSYSLOG_UDP_SENDER_POOL_SIZE]; +static struct SolidSyslogPoolAllocator UdpSender_Allocator = {UdpSender_InUse, SOLIDSYSLOG_UDP_SENDER_POOL_SIZE};🤖 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/SolidSyslogUdpSenderStatic.c` around lines 19 - 21, The three file-scope static variables InUse, Pool, and Allocator must be renamed to follow the class-prefixed static naming rule; change them to descriptive class-prefixed identifiers (for example SolidSyslogUdpSender_InUse or SOLID_SYSLOG_UDP_SENDER_IN_USE for screaming-snake style, SolidSyslogUdpSender_Pool or SOLID_SYSLOG_UDP_SENDER_POOL, and SolidSyslogUdpSender_Allocator or SOLID_SYSLOG_UDP_SENDER_ALLOCATOR) and update every reference and the Allocator initializer (currently referencing InUse and the pool size constant SOLIDSYSLOG_UDP_SENDER_POOL_SIZE) to use the new names so the file-scope statics conform to the Class_Function or CLASS_SCREAMING_SNAKE convention.Core/Source/SolidSyslogSwitchingSenderStatic.c (1)
18-20: ⚡ Quick winRename file-scope statics to class-prefixed identifiers.
InUse,Pool, andAllocatorshould follow the repository’s file-scope static naming convention.As per coding guidelines `**/*.{c,h}`: file-scope statics `Class_Function` or `CLASS_SCREAMING_SNAKE`.Proposed rename
-static bool InUse[SOLIDSYSLOG_SWITCHING_SENDER_POOL_SIZE]; -static struct SolidSyslogSwitchingSender Pool[SOLIDSYSLOG_SWITCHING_SENDER_POOL_SIZE]; -static struct SolidSyslogPoolAllocator Allocator = {InUse, SOLIDSYSLOG_SWITCHING_SENDER_POOL_SIZE}; +static bool SwitchingSender_InUse[SOLIDSYSLOG_SWITCHING_SENDER_POOL_SIZE]; +static struct SolidSyslogSwitchingSender SwitchingSender_Pool[SOLIDSYSLOG_SWITCHING_SENDER_POOL_SIZE]; +static struct SolidSyslogPoolAllocator SwitchingSender_Allocator = { + SwitchingSender_InUse, + SOLIDSYSLOG_SWITCHING_SENDER_POOL_SIZE +};🤖 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/SolidSyslogSwitchingSenderStatic.c` around lines 18 - 20, Rename the file-scope statics to follow the file/class-prefixed naming convention: change InUse to SolidSyslogSwitchingSenderStatic_InUse, Pool to SolidSyslogSwitchingSenderStatic_Pool, and Allocator to SolidSyslogSwitchingSenderStatic_Allocator (and update the initializer to reference SolidSyslogSwitchingSenderStatic_InUse). Update all usages in SolidSyslogSwitchingSenderStatic.c to the new identifiers so the pool, in-use array and allocator follow the Class-prefixed file-scope static naming convention.
🤖 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 `@Core/Source/SolidSyslogOriginSdStatic.c`:
- Line 11: Remove the redundant direct include of SolidSyslogPrival.h from
SolidSyslogOriginSdStatic.c: the file already includes the needed enums via
SolidSyslogError.h (which provides SOLIDSYSLOG_SEVERITY_ERROR and
SOLIDSYSLOG_SEVERITY_WARNING), so delete the `#include` "SolidSyslogPrival.h" line
to avoid the duplicate/transitive include.
In `@Core/Source/SolidSyslogSwitchingSenderStatic.c`:
- Around line 22-35: SolidSyslogSwitchingSender_Create dereferences config
without validation; add a null/field check at the start of
SolidSyslogSwitchingSender_Create (validate config and required fields such as
config->Selector) and if invalid call SolidSyslog_Error with a clear message and
immediately return SolidSyslogNullSender_Get() instead of proceeding; only after
config is validated acquire a pool index and call
SwitchingSender_Initialise(&Pool[index].Base, config), otherwise keep existing
pool-exhausted error path unchanged.
In `@Core/Source/SolidSyslogTimeQualitySdStatic.c`:
- Around line 22-29: The function SolidSyslogTimeQualitySd_Create currently
accepts and stores a NULL getTimeQuality, which later leads to an unguarded
callback invocation; fix this by validating getTimeQuality at the start of
SolidSyslogTimeQualitySd_Create and returning SolidSyslogNullSd_Get()
immediately if it is NULL (i.e., do not call
SolidSyslogPoolAllocator_AcquireFirstFree, TimeQualitySd_Initialise, or assign
into Pool when getTimeQuality is NULL), ensuring only non-NULL callbacks are
stored in Pool and avoiding later crashes when the callback is invoked.
In `@Tests/SolidSyslogSwitchingSenderTest.cpp`:
- Around line 95-105: In the two tests
SolidSyslogSwitchingSender_DestroyDoesNotSendToInnerSenders and
SolidSyslogSwitchingSender_DestroyDoesNotDisconnectInnerSenders, after calling
SolidSyslogSwitchingSender_Destroy(sender) set the test-level sender variable to
nullptr to avoid the teardown path attempting to destroy the same handle twice;
keep the existing CALLED_FAKE_ON assertions for
SenderFake_Send/SenderFake_Disconnect against innerA/innerB unchanged so the
tests still verify no calls were made.
---
Nitpick comments:
In `@Core/Source/SolidSyslogSwitchingSenderStatic.c`:
- Around line 18-20: Rename the file-scope statics to follow the
file/class-prefixed naming convention: change InUse to
SolidSyslogSwitchingSenderStatic_InUse, Pool to
SolidSyslogSwitchingSenderStatic_Pool, and Allocator to
SolidSyslogSwitchingSenderStatic_Allocator (and update the initializer to
reference SolidSyslogSwitchingSenderStatic_InUse). Update all usages in
SolidSyslogSwitchingSenderStatic.c to the new identifiers so the pool, in-use
array and allocator follow the Class-prefixed file-scope static naming
convention.
In `@Core/Source/SolidSyslogUdpSenderStatic.c`:
- Around line 19-21: The three file-scope static variables InUse, Pool, and
Allocator must be renamed to follow the class-prefixed static naming rule;
change them to descriptive class-prefixed identifiers (for example
SolidSyslogUdpSender_InUse or SOLID_SYSLOG_UDP_SENDER_IN_USE for screaming-snake
style, SolidSyslogUdpSender_Pool or SOLID_SYSLOG_UDP_SENDER_POOL, and
SolidSyslogUdpSender_Allocator or SOLID_SYSLOG_UDP_SENDER_ALLOCATOR) and update
every reference and the Allocator initializer (currently referencing InUse and
the pool size constant SOLIDSYSLOG_UDP_SENDER_POOL_SIZE) to use the new names so
the file-scope statics conform to the Class_Function or CLASS_SCREAMING_SNAKE
convention.
🪄 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: 9f12d3e4-c01d-4890-8025-6d03c9698082
📒 Files selected for processing (56)
Bdd/Targets/FreeRtos/main.cBdd/Targets/Linux/main.cBdd/Targets/Windows/BddTargetWindows.cCLAUDE.mdCore/Interface/SolidSyslogMetaSd.hCore/Interface/SolidSyslogNullSd.hCore/Interface/SolidSyslogNullSecurityPolicy.hCore/Interface/SolidSyslogNullSender.hCore/Interface/SolidSyslogNullStore.hCore/Interface/SolidSyslogOriginSd.hCore/Interface/SolidSyslogPassthroughBuffer.hCore/Interface/SolidSyslogSwitchingSender.hCore/Interface/SolidSyslogTimeQualitySd.hCore/Interface/SolidSyslogTunablesDefaults.hCore/Interface/SolidSyslogUdpSender.hCore/Source/CMakeLists.txtCore/Source/SolidSyslogBlockStore.cCore/Source/SolidSyslogErrorMessages.hCore/Source/SolidSyslogMetaSd.cCore/Source/SolidSyslogMetaSdPrivate.hCore/Source/SolidSyslogMetaSdStatic.cCore/Source/SolidSyslogNullSd.cCore/Source/SolidSyslogNullSecurityPolicy.cCore/Source/SolidSyslogNullSender.cCore/Source/SolidSyslogNullStore.cCore/Source/SolidSyslogOriginSd.cCore/Source/SolidSyslogOriginSdPrivate.hCore/Source/SolidSyslogOriginSdStatic.cCore/Source/SolidSyslogPassthroughBuffer.cCore/Source/SolidSyslogPassthroughBufferPrivate.hCore/Source/SolidSyslogPassthroughBufferStatic.cCore/Source/SolidSyslogSwitchingSender.cCore/Source/SolidSyslogSwitchingSenderPrivate.hCore/Source/SolidSyslogSwitchingSenderStatic.cCore/Source/SolidSyslogTimeQualitySd.cCore/Source/SolidSyslogTimeQualitySdPrivate.hCore/Source/SolidSyslogTimeQualitySdStatic.cCore/Source/SolidSyslogUdpSender.cCore/Source/SolidSyslogUdpSenderPrivate.hCore/Source/SolidSyslogUdpSenderStatic.cDEVLOG.mdTests/Bdd/Targets/BddTargetServiceThreadTest.cppTests/CMakeLists.txtTests/SolidSyslogBlockStoreDrainOrderingTest.cppTests/SolidSyslogMetaSdTest.cppTests/SolidSyslogNullSdTest.cppTests/SolidSyslogNullSecurityPolicyTest.cppTests/SolidSyslogNullSenderTest.cppTests/SolidSyslogNullStoreTest.cppTests/SolidSyslogOriginSdTest.cppTests/SolidSyslogPassthroughBufferTest.cppTests/SolidSyslogPosixMessageQueueBufferTest.cppTests/SolidSyslogSwitchingSenderTest.cppTests/SolidSyslogTest.cppTests/SolidSyslogTimeQualitySdTest.cppTests/SolidSyslogUdpSenderTest.cpp
cppcheck flagged the SwitchingSenderPool TEST_GROUP's `config = {}`
followed by an immediate overwrite in setup() as `redundantInitialization`.
Drop the explicit `= {}` and let setup() be the single initialisation site.
cppcheck also flagged TimeQualitySdPool's `pooled` and `overflow` because
that fixture has no setup() touching other members, so cppcheck cannot
see TEST() bodies mutating them. Add per-line suppressions noting the
same "cppcheck does not model CppUTest macros" reason used elsewhere
in the project.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Public headers (UdpSender.h, SwitchingSender.h) gain a forward declaration of `struct SolidSyslogSender` — the new _Destroy(struct SolidSyslogSender*) parameter type was relying on the caller transitively pulling SolidSyslogSenderDefinition.h. Static.c files drop their direct includes of the Definition headers where a forward declaration suffices (Sender/StructuredData) and add the explicit forward decls IWYU requests. TimeQualitySdStatic.c gains SolidSyslogTimeQuality.h for SolidSyslogTimeQualityFunction (it was arriving transitively from the now-dropped Definition header chain). NullSd.c gains `struct SolidSyslogFormatter;` forward decl. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit (correctly) flagged that SolidSyslogSwitchingSender_Create dereferenced config without validating it — a NULL config or NULL Selector would crash inside _Initialise. Add the same bad-config contract UdpSender already had: NULL config, NULL config->Senders, or NULL config->Selector each emit a SOLIDSYSLOG_SEVERITY_ERROR with a descriptive message and route to SolidSyslogNullSender_Get(). Three new SolidSyslogSwitchingSenderBadSetup tests in SolidSyslogSwitchingSenderTest.cpp exercise each branch, plus one SendOnBadSetupSenderReturnsTrueAndDrops to pin the contract that bad-setup messages are dropped at the null-object boundary rather than retained in the Store. 100% line coverage preserved on SwitchingSenderStatic.c. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit flagged that SolidSyslogTimeQualitySd_Create accepted a NULL getTimeQuality callback, which then crashes inside TimeQualitySd_Format when the callback is dereferenced. Discussed severity choice: ERROR rather than WARNING because the integrator explicitly called _Create with no callback at all (programmer contract violation), matching the bad-setup pattern in feedback_bad_setup_contract.md. NULL callback → emits SOLIDSYSLOG_SEVERITY_ERROR with a descriptive message and routes to SolidSyslogNullSd_Get(). The pool migration is the natural place to add this safety because we now have a fallback Null object to return; the original singleton couldn't gracefully degrade. One new SolidSyslogTimeQualitySdBadSetup test exercises the branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DestroyDoesNotSendToInnerSenders and DestroyDoesNotDisconnectInnerSenders explicitly call _Destroy on the fixture's sender. Under pool semantics, teardown's _Destroy(sender) would then operate on a stale handle and emit the SOLIDSYSLOG_ERROR_MSG_SWITCHINGSENDER_UNKNOWN_DESTROY warning. Apply the destroy-before-recreate pattern: explicitly destroy, null the fixture pointer to suppress the CreateSwitchingSender guard's re-destroy, assert the inner-sender contracts, then re-create so teardown's destroy hits a live handle. Every Destroy in the test path now targets a live, in-pool handle. Test-only — production code unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit (correctly) flagged that the InUse / Pool / Allocator file-scope statics in the new Static.c TUs lacked the class prefix required by NAMING.md (Tier 2: file-scope statics are Class_Function or CLASS_SCREAMING_SNAKE). Same gap exists in the pre-existing CircularBufferStatic.c — including it in the sweep keeps the project consistent under NAMING.md after S11.04 rather than leaving a single non-conformant file. All seven E11 Static.c files renamed: InUse → <Class>_InUse Pool → <Class>_Pool Allocator → <Class>_Allocator where <Class> is the file's class basename. Affected: CircularBuffer, PassthroughBuffer, UdpSender, SwitchingSender, MetaSd, TimeQualitySd, OriginSd. Out of scope: the `Fallback` struct + Fallback_Read / Fallback_Write functions in CircularBuffer and PassthroughBuffer. Same NAMING.md concern; CR didn't flag them; deferring to a focused cleanup. Also: drop the value-init from config in SwitchingSenderBadSetup fixture (cppcheck redundantInitialization), and suppress unreadVariable on the setup() assignment (cppcheck doesn't model the TEST() macro reads of fixture members). No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
CodeRabbit response summaryAll four actionable comments addressed and all six checks now green locally (debug, sanitize, coverage, tidy, cppcheck, iwyu, format). 5 fix commits pushed:
Plus two prerequisite CI fixes (cppcheck Carrying forward into memory: a feedback note that production-code review fixes need explicit approval before applying (David caught me about to push a behaviour change with a debatable severity choice without discussing it first). |
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1159 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
This was referenced May 20, 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 #399. Applies the canonical E11 three-TU split +
SolidSyslogPoolAllocatorpattern (established by S11.02) to the six Core stateful classes that didn't have a storage cast:PassthroughBuffer,UdpSender,SwitchingSender,MetaSd,TimeQualitySd,OriginSd. Eliminates the file-scope-singleton + config-args pattern that silently mutated a shared instance on second_Create— every migrated class now draws from a library-internal static pool with a fallback singleton on exhaustion.Change Description
SolidSyslogNullSd(used as fallback by all three migrated SDs) andSolidSyslogNullSender(used by UdpSender and SwitchingSender; will also be reused by S11.05's StreamSender). Both are_Get-only — single immutable shared instance, no_Create/_Destroylifecycle.NullStoreandNullSecurityPolicyflipped to the same_Get-only shape. Their_Create/_Destroylifecycle was vestigial — the instance was always static. Standardising on_Getfor null/immutable types.SolidSyslogPoolAllocatorfollowing the canonical 3-TU split (Class.c + ClassPrivate.h + ClassStatic.c). Per-classStatic.cends at the documented target shape (_Create,_Destroy,IndexFromHandle+ theCleanupAtIndexbridge)._Destroy(void)→_Destroy(struct <Base>* base). The handle returned from_Createis now the argument to_Destroy. Parameter namebasefollows NAMING.md's this-pointer rule.Sendreturnstrue(drop on the floor) rather thanfalse. The old class-privateNilUdpSenderalready did this for a reason — a misconfigured Sender paired with a real Store would otherwise accumulate undeliverables. SwitchingSender's out-of-range-selector path adopts the same semantic via the shared NullSender; twoReturns_Falsetests flip toReturns_Trueaccordingly.SOLIDSYSLOG_<CLASS>_POOL_SIZE, default1U, floor1) added toSolidSyslogTunablesDefaults.h. Override viaSOLIDSYSLOG_USER_TUNABLES_FILE.SolidSyslogErrorMessages.hforPOOL_EXHAUSTED(ERROR severity) andUNKNOWN_DESTROY(WARNING severity).udpSenderandswitchingSenderpointers so theDestroySender()function can pass handles back into_Destroy._Destroyparameter names tobaseeverywhere per NAMING.md, after I caught having used the friendly names mid-stream.Test Evidence
_Destroy(void)callers swapped to_Destroy(handle).FillingPoolThenOverflowReturnsDistinctFallback(andFallbackWriteAndReadAreNoOpsfor PassthroughBuffer). Proves the tunable caps live instances and overflow returns a distinct fallback. Generic pool mechanics (lock counts, per-probe locking, stale-handle warning) stay covered bySolidSyslogPoolAllocatorTest.cpp— no need to copy the 9-test set per class.SOLIDSYSLOG_<CLASS>_POOL_SIZE=3run for every migrated class via the user-tunables override mechanism — full suite green at non-default sizing.main.cpputest-freertoscontainer perfeedback_verify_in_freertos_host_image.md—LayerGuarddoesn't silently skipTests/FreeRtos/Platform/FreeRtoshere.Areas Affected
Core/Interface/:SolidSyslogNullSd.h,SolidSyslogNullSender.h(new);SolidSyslogNullStore.h,SolidSyslogNullSecurityPolicy.h,SolidSyslogPassthroughBuffer.h,SolidSyslogUdpSender.h,SolidSyslogSwitchingSender.h,SolidSyslogMetaSd.h,SolidSyslogTimeQualitySd.h,SolidSyslogOriginSd.h,SolidSyslogTunablesDefaults.h(modified).Core/Source/:SolidSyslogNullSd.c,SolidSyslogNullSender.c, plus<Class>Private.h+<Class>Static.cfor each of the six migrated classes (new);SolidSyslogNullStore.c,SolidSyslogNullSecurityPolicy.c, the six<Class>.cfiles,SolidSyslogErrorMessages.h,CMakeLists.txt(modified).Tests/: Six existing test files updated;SolidSyslogNullSdTest.cppandSolidSyslogNullSenderTest.cppadded;SolidSyslogSwitchingSenderTest.cpptest fixture gained a defensive Destroy inCreateSwitchingSenderto handle the override pattern under pool semantics.Bdd/Targets/{Linux,Windows,FreeRtos}/: destructor pairs updated; file-scopeudpSenderandswitchingSenderpointers added.CLAUDE.md: audience-table rows updated for every migrated class + the two new GoF nulls.DEVLOG.md: closing entry for S11.04.🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Bug Fixes
Documentation
Tests