refactor: S24.17 collapse per-platform pool tunables to role-based#504
Merged
Merged
Conversation
Reduce the pool-size tunable surface in SolidSyslogTunablesDefaults.h from 35 macros to 18 by collapsing 8 platform/vendor-selected role groups to a single role-named tunable each: TCP_STREAM <- POSIX/WINSOCK/PLUS_TCP/LWIP_RAW TCP stream DATAGRAM <- POSIX/WINSOCK/PLUS_TCP/LWIP_RAW datagram RESOLVER <- GETADDRINFO/WINSOCK/PLUS_TCP/LWIP_RAW/LWIP_RAW_DNS MUTEX <- POSIX/WINDOWS/FREE_RTOS mutex FILE <- POSIX/WINDOWS/FATFS file ATOMIC_COUNTER <- STD/WINDOWS atomic counter TLS_STREAM <- OpenSSL/mbedTLS TLS stream HMAC_SHA256_POLICY <- OpenSSL/mbedTLS HMAC policy A build links exactly one implementation per role, so the integrator tunes the role (SOLIDSYSLOG_TCP_STREAM_POOL_SIZE) rather than the platform. Extends the existing SOLIDSYSLOG_ADDRESS_POOL_SIZE precedent; the table now grows O(1) per new OS/stack/vendor instead of O(roles). Within each role group the prior per-platform defaults were already uniform, so no default-value reconciliation. Clean break, no deprecated aliases (pre-1.0). All floor static_asserts kept. Convention + the two-implementations-in-one-build sum caveat documented in docs/NAMING.md and the header preamble. No behaviour change. Closes #501 Co-Authored-By: Claude Opus 4.8 (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 (59)
📝 WalkthroughWalkthroughThis PR consolidates 35 platform/vendor-specific pool-size tunables into 8 role-based tunables across the SolidSyslog codebase, eliminating redundant per-platform naming (POSIX, Windows, FreeRTOS, lwIP, etc.) in favor of unified role-based constants. The change includes mechanical updates to ~50 platform implementations and tests, documentation of the role-based convention, and no behavioral changes. ChangesPool-Size Tunable Role-Based Architecture
Platform Static Pool Implementation Updates
Pool Allocation and Exhaustion Test Updates
Board Definition and Test Configuration Updates
Convention Documentation and Integration Guides
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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 (✔️ 1376 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The pool-size tunable surface in
Core/Interface/SolidSyslogTunablesDefaults.hnamed the same role once per platform/vendor implementation (SOLIDSYSLOG_POSIX_TCP_STREAM_POOL_SIZE,_WINSOCK_TCP_STREAM_,_PLUS_TCP_TCP_STREAM_,_LWIP_RAW_TCP_STREAM_— four names for "how many TCP streams"). A build links exactly one implementation per role, so the integrator had to know their platform to find the knob that mattered, and the table grew O(roles) per new OS/stack/crypto vendor.SOLIDSYSLOG_ADDRESS_POOL_SIZEalready proved the role-based model across three platform Address types; this extends it consistently.Closes #501.
Change Description
Collapsed 8 platform/vendor-selected role groups to a single role-named tunable each (clean break, no deprecated aliases — pre-1.0):
SOLIDSYSLOG_TCP_STREAM_POOL_SIZESOLIDSYSLOG_DATAGRAM_POOL_SIZESOLIDSYSLOG_RESOLVER_POOL_SIZESOLIDSYSLOG_MUTEX_POOL_SIZESOLIDSYSLOG_FILE_POOL_SIZESOLIDSYSLOG_ATOMIC_COUNTER_POOL_SIZESOLIDSYSLOG_TLS_STREAM_POOL_SIZESOLIDSYSLOG_HMAC_SHA256_POLICY_POOL_SIZENet 35 → 18 pool tunables. Within each role group the prior per-platform defaults were already uniform, so no default-value reconciliation was needed. All floor
static_asserts preserved. Pure rename + header consolidation — no behaviour change.The role rule ("pool counts instances, not implementations; if two implementations of one role ever coexist in a build, size to the sum") is documented in the header preamble and codified in
docs/NAMING.md. A pre-implementation audit (recorded on #501) confirmed no current executable wires two implementations of one role, so every existing override maps 1:1 to a role override at the same value — the fixtureSmallMessageSizeTunables.heven collapsed its two atomic-counter lines into one.Test Evidence
Not TDD — mechanical refactor with full existing coverage. Verified locally:
debug(gcc): 1352 tests pass.clang-debug: 1352 tests pass.freertos-hostdebug(the only image that compiles the FreeRTOS/PlusTcp/LwipRaw/MbedTls/FatFs trees this touches): all 12 suites pass (1352 + 11 platform suites).clang-format --dry-run --Werrorclean on all 56 changed source files (reflowed the pool-allocator initialisers whose alignment shifted with the new token lengths).Areas Affected
Core/Interface/SolidSyslogTunablesDefaults.h— 25 macro blocks consolidated into 8 role blocks (−419/+110 lines).Platform/**/*Static.c— pool arrays reference the role macro.Tests/**— pool/exhaustion tests reference the role macro;Tests/Fixtures/SmallMessageSizeTunables.htwo atomic-counter lines → one.Bdd/Targets/FreeRtos{,Lwip}/solidsyslog_user_tunables.h—FREE_RTOS_MUTEX→MUTEX.CLAUDE.md,docs/NAMING.md(new convention + sum caveat),docs/integrating-lwip.md(merged resolver rows),docs/misra-deviations.md(stale identifier example).No impact on the public API surface (
SolidSyslog*.haudience headers) beyond the documented tunable names. Integrators with a customSOLIDSYSLOG_USER_TUNABLES_FILEthat overrode a per-platform pool name must rename to the role name.🤖 Generated with Claude Code
Summary by CodeRabbit
Configuration Changes
SOLIDSYSLOG_MUTEX_POOL_SIZEinstead of platform-specific variants). Users must update pool-size tunable names in their configurations to use the new role-based naming convention.Documentation