Make the compare headers self-contained (move comparison_error to its own header) - #51
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #51 +/- ##
========================================
Coverage 81.90% 81.90%
========================================
Files 22 23 +1
Lines 923 923
Branches 373 373
========================================
Hits 756 756
Misses 160 160
Partials 7 7
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@Lastique Would you please review this - may seem lots of file - but I think I may have a point |
| # Each compare header must compile on its own (clang rejects them otherwise). | ||
| foreach(h certain possible lexicographic set tribool) | ||
| boost_test(TYPE compile SOURCES compare_${h}_self_contained.cpp) | ||
| endforeach() | ||
|
|
There was a problem hiding this comment.
I'd say every public header must be self-contained, not just compare headers.
Instead of listing headers and duplicating the test .cpp, use a glob and a single tes .cpp. For example: Jamfile, .cpp. I don't have a CMakeLists.txt example at hand, but it should be easy to do with file(GLOB).
Note that the self-contained tests are better be disabled by default. Enable them only in one or two jobs in CI. These tests tend to take a long time, especially on Windows, and there's no point in running them in every CI configuration. (Do note that the tests will run each time for debug/release and static/dynamic link, i.e. 4 times, if your CI job requests that.)
compare/{certain,possible,lexicographic,set,tribool}.hpp throw comparison_error, but only see its
forward declaration in detail/interval_prototype.hpp. Clang rejects them when they are included standalone (incomplete type 'comparison_error'); g++ accepts them. These
compile tests fail on clang until that is fixed.
…self-contained
The compare headers throw comparison_error but only saw its forward declaration in detail/interval_prototype.hpp; the definition lived in interval.hpp. Move it to detail/comparison_error.hpp and include that from interval.hpp and from compare/{certain,possible,lexicographic,set,tribool}.hpp. The class itself is unchanged.
…ed in one b2 and one CMake CI job)
5b0467a to
aa48982
Compare
2 participants
The
compare/*.hppheaders (certain, possible, lexicographic, set, tribool) throwcomparison_error, but they only see the forward declaration indetail/interval_prototype.hpp.The actual class is defined in
interval.hpp, which they don't include. Sincethrow comparison_error();doesn't depend on the template parameters, clang checks it right away and fails if the class hasn't been defined yet.g++ doesn't complain.
In practice you would hit the issue with
compare/tribool.hpp. The docs list it as an extension thatinterval.hppdoesn't include, so users include it themselves. If it ends up aboveinterval/interval.hpp(which is what alphabetical include sorting gives you), clang fails:compare/certain.hppalonecompare/certain.hpp, theninterval.hppinterval.hpp, thencompare/certain.hppcompare/tribool.hpp, theninterval/interval.hppChanges
nothing changes for code that already works.
comparison_erroris markedBOOST_SYMBOL_VISIBLEso it can be caught across shared library boundaries with older gcc versions.ext/x86_fast_rounding_control.hppnow includes what it uses (rounding.hppanddetail/x86_rounding_control.hpp). It was the only other public header that didn't compile on its own.Self-contained header tests for every public header. A single
test/self_contained_header.cppis compiled once per header.Both b2 (
path.glob-tree) and CMake (file(GLOB_RECURSE)) glob the headers and skipdetail/.ext/x86_fast_rounding_control.hppis x86-only by design, so its test only runs on x86: b2 usescheck-target-builds /boost/architecture//x86and CMake checksCMAKE_SYSTEM_PROCESSOR.The tests are off by default. Set the
BOOST_NUMERIC_INTERVAL_TEST_SELF_CONTAINED_HEADERSenvironment variable for b2, or-DBOOST_NUMERIC_INTERVAL_TEST_SELF_CONTAINED_HEADERS=ONfor CMake.CI turns them on in one b2 job (clang-22) and one CMake job (ubuntu, shared, Debug).