Codebase review fixes: CMake, tests, docs, include hygiene#34
Merged
Conversation
DNedic
force-pushed
the
claude/codebase-review-ZpEci
branch
2 times, most recently
from
3397e58 to
2354747
Compare
Previously CMake added the bare `LOCKFREE_CACHE_COHERENT` token to compile definitions whenever the variable was defined, which the compiler treated as `-DLOCKFREE_CACHE_COHERENT=1` regardless of the value the user set. `-DLOCKFREE_CACHE_COHERENT=false` thus silently kept cacheline padding enabled, leaving embedded targets wasting memory despite the README's instruction to disable it. Replace the variable with a proper `option()` defaulting to ON and forward it through `$<BOOL:...>` so any of CMake's truthy values enable padding and any falsy value (`OFF`, `false`, `0`, ...) actually disables it.
`cnt << 2 + prio` parses as `cnt << (2 + prio)` because + binds tighter than <<, so the priority bits never landed in the low two bits of the produced value. The downstream verification masked the low two bits to recover the priority and therefore always saw 0, making the priority ordering check a no-op. Use `(cnt << 2) | prio` to match the documented intent.
The producer pushed TEST_MT_TRANSFER_CNT + 1 elements while the consumer's exit condition was driven by a counter that stopped at TEST_MT_TRANSFER_CNT, leaving one element abandoned in the queue. Unlike the spsc::Queue test (which terminates by observing the last value popped) the priority queue test counts pops, so the +1 is unnecessary.
…ision The release count for test_data2 was computed using sizeof(test_data[0]). It happened to produce the right number because both arrays are uint32_t, but the expression is wrong and a hazard for anyone copy/pasting from the tests.
The badge URL had `.github/workflows/` duplicated, so the badge returned 404. GitHub Actions badges take only the workflow filename under `actions/workflows/`.
The Push/Pop comments were copy-pasted from the spsc::PriorityQueue header and told users to call them only from a single producer or consumer thread. The mpmc variant supports multiple producers and consumers, which is the entire reason it exists.
Read, Peek and Skip all advertised "Write success" as their return value, copy-pasted from Write. Make them describe what they actually return.
Doxygen requires the parameter name as the first token of @param to associate the description with the right argument. The library's public headers consistently omitted the name, so generated docs ended up with detached descriptions.
Two of the example snippets had a missing closing paren after ADC_PollDmaComplete and a stray extra paren after read.empty(), making the examples uncompilable as written.
DNedic
force-pushed
the
claude/codebase-review-ZpEci
branch
from
2354747 to
9646865
Compare
The verification it performed couldn't actually check priority ordering under concurrent producer/consumer: Pop scans sub-queues from highest to lowest priority, so a higher-priority Push that lands in a sub-queue the scan has already passed legitimately leaves a higher-priority value behind while a lower-priority value is returned. With that strict check removed only value-integrity remained, which spsc::Queue's multithreaded test already covers via the per-priority sub-queues.
The umbrella `lockfree.hpp` defaulted `LOCKFREE_CACHE_COHERENT` and `LOCKFREE_CACHELINE_LENGTH` via `#ifndef`. That had two problems: the values weren't used when consumers pulled in an individual data structure header without going through `lockfree.hpp` first (silently disabling cacheline padding and breaking alignment), and the defaults duplicated logic that now lives in CMake. Move all defaulting into CMake: `LOCKFREE_CACHELINE_LENGTH` becomes a cache STRING defaulting to 64, and the two macros are always forwarded as compile definitions. Drop the umbrella defaults and add an `#error` to each data structure header so a build that forgets to define them fails loudly instead of silently producing the wrong layout. Non-CMake build systems must now define both macros themselves.
DNedic
force-pushed
the
claude/codebase-review-ZpEci
branch
from
9646865 to
9986038
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
A batch of correctness, documentation and build-config fixes uncovered during a codebase review. Each commit is independently revertable.
Correctness
fix(cmake): forward LOCKFREE_CACHE_COHERENT value— the option was passed as a bare definition (-DLOCKFREE_CACHE_COHERENT) which the preprocessor interpreted as1regardless of the value supplied to CMake, so-DLOCKFREE_CACHE_COHERENT=falsewas silently ignored. Now forwarded the same wayLOCKFREE_CACHELINE_LENGTHalready is.fix(test/spsc/priority_queue): correct value-priority encoding—cnt << 2 + prioparses ascnt << (2 + prio)because+binds tighter than<<, so the priority bits never landed in the low two bits of the produced value. The downstream verification masked the low two bits to recover the priority and therefore always saw0, making the priority-ordering check a no-op. Replaced with(cnt << 2) | prio.fix(test/spsc/priority_queue): match producer/consumer counts— producer pushedTEST_MT_TRANSFER_CNT + 1items but the consumer's exit condition was a counter that stopped atTEST_MT_TRANSFER_CNT, leaving one element abandoned in the queue.fix(test/spsc/bipartite_buf): use the right array in element-size division—sizeof(test_data2) / sizeof(test_data[0])mixed the two arrays. Numerically equivalent only because both areuint32_t.fix(readme): correct CMake badge URL— the badge URL had.github/workflows/duplicated and 404'd.Build hygiene
fix(cmake): stop exposing spsc/ and mpmc/ as include directories— the two subdirectories were on the interface include path so a consumer could resolve<queue.hpp>to lockfree's queue header, risking silent shadowing of other libraries' headers. None of the headers needed it.Documentation
fix(doc/mpmc/priority_queue): remove single-thread caveats— Push/Pop comments were copy-pasted from spsc and told users to call them only from a single producer/consumer thread. The MPMC variant supports multiple of each by definition.fix(doc/spsc/ring_buf): correct @retval text for Read/Peek/Skip— Read, Peek and Skip all advertisedWrite success.doc(lockfree): add parameter names to Doxygen @param tags— public headers consistently omitted the param name, so generated docs ended up with detached descriptions.doc(spsc/bipartite_buf): fix unbalanced parentheses in examples— twoif (ADC_PollDmaComplete(&adc_dma_h) {and oneif (!read.empty())).doc(priority_queue): fix "chosing" typoin both spsc and mpmc priority queue docs.doc(changelog): close unmatched code span in 2.0.10 entry— stray opening backtick.Test plan
cmake -B build && cmake --build buildcleanctest --output-on-failure --test-dir build/tests— all 62 tests passhttps://claude.ai/code/session_01CnTyrUBx15RvNeuATVJyWY
Generated by Claude Code