fix: Stop the iterator reading one limb past the end

[chenpeizhi]

Two defects in the header, both present since long before the C++20 migration.

The iterator reads one limb past the end

Fbitset::const_iterator::get_next advances the limb pointer and then dereferences it, and only afterwards re-tests it against the sentinel:

while (curr_ < last_ && limb_ == 0) {
    ++curr_;
    limb_ = *curr_;      // curr_ may already be last_ here
    base_ += LIMB_BITS;
}

When the final limb is exhausted, ++curr_ makes curr_ == last_ and the read goes one limb past the end of the storage. The value read is discarded immediately, because the loop condition then fails, so iteration results are correct. The read itself is still undefined behaviour, and it is reached by every complete walk over a bit set, so it happens constantly.

Found while running libparenth under the address sanitizer:

ERROR: AddressSanitizer: stack-buffer-overflow
    #0 fbitset::Fbitset<1ul, unsigned long long, fbitset::No_ext>::const_iterator::get_next() fbitset.hpp:531
    #1 ... ::const_iterator::operator++() fbitset.hpp:517
    #2 libparenth::Parenther<...>::form_chunks(...) libparenth.hpp:818

The fix reads the limb only after the bound check.

The header cannot be included from two translation units

is_no_ext is a variable template whose explicit specialization was declared without inline:

template <typename T> constexpr bool is_no_ext = false;
template <> constexpr bool is_no_ext<No_ext> = true;

An explicit specialization does not inherit the internal linkage that constexpr gives the primary template, so it is emitted in every translation unit that includes the header. Any program including fbitset.hpp twice fails to link:

duplicate symbol 'fbitset::internal::is_no_ext<fbitset::No_ext>' in:
    .../nonclassical.cpp.o
    .../matrixchain.cpp.o

For a header-only library that is a hard restriction on how it can be used. Nothing had hit it because every consumer so far included the header from exactly one translation unit. Both declarations are now inline.

Tests

test/multi_tu.cpp is a second translation unit including the header, which covers the linkage. It also walks a bit set to exhaustion in every supported configuration, which is the path that used to read past the end.

The out-of-bounds read is invisible to a plain test run, since the value is discarded. So there is also a new CI job that builds the tests with the address and undefined behaviour sanitizers. Reverting only the get_next change makes that job fail, and the plain jobs still pass, which is exactly why the job is needed.

Effect on results

None. This was checked by fingerprinting the full output of the libparenth search (every intermediate, its parenthesization and its cost) over 500 randomly generated contraction problems in all six mode and inclusivity combinations, before and after. All 2861 configurations that completed give byte-identical results.

[Copilot]

Pull request overview

Fixes long-standing undefined behavior in Fbitset iteration (one-past-the-end limb read) and resolves a header-only ODR/linkage issue with a variable-template specialization. Adds a multi-translation-unit test and a sanitizer CI job to reliably catch the previously invisible UB.

Changes:

• Fix Fbitset::const_iterator::get_next() to avoid dereferencing curr_ after it reaches the sentinel.
• Make internal::is_no_ext (primary + explicit specialization) inline constexpr to prevent duplicate symbols across translation units.
• Add a second-TU test and a new GitHub Actions job that runs tests under ASan/UBSan.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
include/fbitset.hpp	Fixes iterator UB and makes is_no_ext safe for header-only multi-TU inclusion.
test/multi_tu.cpp	Adds coverage for multi-TU header inclusion and iterator exhaustion paths.
test/CMakeLists.txt	Builds the new second translation unit into the test executable.
.github/workflows/ci.yml	Adds a sanitizer job to detect UB that regular test runs won’t observe.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chenpeizhi]

Right on both counts, and dropped in e7793a0.

This translation unit never checks an Fbitset directly, so it does not need the stream operator at all. test/basic.cpp keeps its copy, which is now the only one.

Two copies of the same template in separate translation units is exactly the hazard this pull request is about, so removing one is better than keeping them in step by hand. If a third unit ever needs it, the definition should move to a shared test header rather than be copied again.

Tests still pass in both the plain and the sanitizer builds.