Codebase review fixes: CMake propagation, asserts, docs, style#3
Merged
Conversation
DNedic
commented
May 19, 2026
| add_subdirectory(lfbb) | ||
|
|
||
| # Library configuration | ||
| # These are exposed as PUBLIC because they affect the layout of LFBB_Inst_Type |
Owner
Author
There was a problem hiding this comment.
This comment is unnecessary.
Owner
Author
There was a problem hiding this comment.
DNedic
pushed a commit
that referenced
this pull request
May 19, 2026
Addresses review feedback on #3.
…s PUBLIC The previous CMake code added a bare `-DLFBB_MULTICORE_HOSTED` regardless of the value the user passed, so `cmake -DLFBB_MULTICORE_HOSTED=false` would still enable multicore mode (the bare macro expands to 1). The defines were also PRIVATE, which meant the struct layout in the header differed between the library build and consumer builds — a silent ABI mismatch when either option was set. Both defines are now PUBLIC and forward the user-supplied value.
LFBB_WriteRelease already asserts `w + written <= inst->size` to catch callers releasing more than they acquired. Add the symmetric check on the read side so misuse is caught in debug builds rather than silently corrupting the read index.
A size of 1 produces a buffer with zero usable capacity because the implementation always reserves one slot to distinguish empty from full. Tighten the assert from `size != 0` to `size > 1` so the degenerate case is caught at init time.
- README: correct the malformed CI badge URL (duplicated path segment) - README: fix syntax error in the producer code example (missing ')') - README: replace inline triple-backtick spans with single backticks - README: hyphenate "single-producer, single-consumer" - README: spell "phenomenon" correctly - CHANGELOG: spell "inferring" correctly - tests: spell "overflow" correctly in comments - lfbb.h: name parameters in @param tags so Doxygen can link them - lfbb.h: document the NULL return path for LFBB_WriteAcquire and LFBB_ReadAcquire
The rest of the file consistently uses the `U` suffix for size_t literals; this assignment to `*available` was the lone exception.
These are plain markdown files; the executable bit was meaningless and caused them to show up oddly in file managers and `ls -l` output.
v15 dates from early 2023. Move to a current stable release so the pre-commit hook tracks a modern clang-format with recent fixes.
DNedic
force-pushed
the
claude/codebase-review-cHBJx
branch
from
0a4104e to
058d513
Compare
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
2 participants
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.
Summary
Fixes from a code review pass, grouped into one commit per concern (documentation fixes consolidated into a single commit).
Correctness
fix: propagate LFBB_MULTICORE_HOSTED value and expose configuration as PUBLIC— The previous CMake code added a bare-DLFBB_MULTICORE_HOSTEDregardless of the value the user passed, socmake -DLFBB_MULTICORE_HOSTED=falsewould still enable multicore mode (the bare macro expands to1). The defines were alsoPRIVATE, which meant the struct layout in the header differed between the library build and consumer builds — a silent ABI mismatch when either option was set. Both defines now forward the user-supplied value and arePUBLIC.fix: assert read amount stays within buffer in LFBB_ReadRelease—LFBB_WriteReleasealready assertsw + written <= inst->size; add the symmetric check on the read side so misuse is caught in debug builds.fix: reject single-byte buffers in LFBB_Init— A size of 1 produces zero usable capacity because one slot is always reserved to distinguish empty from full. Tighten the assert fromsize != 0tosize > 1.Documentation (single commit)
.github/workflows/duplicated in the path.)inif (ADC_PollDmaComplete(&adc_dma_h) {.lfbb.h@paramtags now include parameter names so Doxygen can link them.LFBB_WriteAcquireandLFBB_ReadAcquirenow document their NULL return paths.Style / housekeeping
style: use 0U for size_t literal in LFBB_ReadAcquire— only place in the source still using bare0for asize_tassignment.chore: drop executable bit on README.md and CHANGELOG.md— they were tracked as100755for no reason.chore: bump pre-commit clang-format mirror from v15.0.7 to v19.1.7— v15 dates from early 2023.Test plan
cmake -B build && cmake --build build && ctest— all 9 tests passcmake -B build -DLFBB_MULTICORE_HOSTED=truenow emits-DLFBB_MULTICORE_HOSTED=trueinC_DEFINES(previously emitted bare-DLFBB_MULTICORE_HOSTED)lfbbvia CMake now inherit the multicore/cacheline defines (manual)Generated by Claude Code