modules: land rpp.sprint and rpp.task, fix the TSAN hook, and move the traits to concepts - #73
Merged
Merged
Conversation
B6 blamed the libc++ spelling of the C15 pattern, and the cause is one attribute. gcc links libtsan.so, which reads __tsan_default_suppressions through the global dynamic symbol table. -fvisibility=hidden kept the definition out of it, so dlsym answered with the weak hook inside libtsan.so, which returns null. Every pattern was dead. clang links its runtime statically, which is why C15 worked there. The hook and its patterns moved to tests/test_sanitizers.cpp, beside a test which calls dlsym(RTLD_DEFAULT, ...) and reads the string back. That test fails without the attribute, and it names the exact property CI lost. The pattern list grew to cover what the gcc jobs report. Both reports on master name test_future::test_except_handler_chaining, which shares one exception object between two pool workers. The refcount which orders them sits in an uninstrumented libstdc++.so, so TSAN sees no edge and calls the free a race. Measured on 2 pinned cores: 3 reports in 120 runs before, 0 in 120 after, and the two patterns matched 3 times each. Ten full-suite runs matched no suppression at all, which is what B6 asked for. One report survived the suppression, and it is a different defect. A pool worker reads its semaphore after the pool destroyed it, so BUGS.md B10 carries it. Verified: gcc TSAN 553/553, gcc 553/553, clang 532/532, clang-tidy reports nothing, and all five gates report no finding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PJQak2qMQ4cXHtqEcpwimN
B8 said gcc-14 writes a module for sprint.h and task.h that no importer reads, and it named a compiler defect with no shape. Both modules ship now. A bisect outside cmake, which builds every .cppm into one gcm.cache and answers in seconds, found two shapes and one workaround each. Shape 1 reaches rpp.sprint through rpp.type_traits. An export which names a function in the std::__cxx11 inline namespace makes the module unreadable. std::to_string and std::stoi both do it, and std::swap does not. The form does not matter. An is_detected_v alias, a plain alias template and a C++20 concept all fail the same way, and so does a concept which calls an unexported helper that names it. Only dropping the export works. So the generator gains NO_EXPORT, and has_std_to_string leaves the module surface. A header includer still gets the trait. std_to_string_expression is gone, because the concept replaced the alias it fed. The concept keeps the old expression, so a class with a non-const conversion operator still matches, and test_sprint.cpp pins all eight cases. Shape 2 reaches rpp.task. A module which includes future_types.h in its global module fragment and also imports rpp.future_types writes an unreadable .gcm. Either half alone is fine, and the importer only fails when it also includes <rpp/tests.h>. NO_IMPORT drops that one re-export. Two smaller fixes ride along. The generator read every header in the bare metal configuration and stopped on a fatal parse, so sprint.h could not generate at all. A header which does not compile in a configuration now drops that guard, because no guard per name saves a header a whole configuration rejects. RppMaskedModuleOnly links the library, because sprint.cpp holds the out-of-line half of rpp.sprint. The generator selftest pins both knobs and the export import a working toolchain writes. test_modules.cpp takes a case per module, and masked_module_only.cpp drives the sprint surface with no header at all. Verified: the modules build passes 556/556, the header build passes 533/533, the TSAN build passes 556/556, the module consumer exits 0 on the modules path, all five gates report no finding, and clang-tidy reports nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PJQak2qMQ4cXHtqEcpwimN
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b2db16ed1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ReCpp demands C++20, and config.h carries a static_assert which proves it. So every trait built on is_detected_v became a concept, and every enable_if_t site became a requires clause. src/rpp carries neither spelling now. type_traits.h drops eight expression aliases and turns seven traits into concepts: has_to_string, has_to_string_memb, has_get_memb, has_set_memb, is_iterable, is_stringlike and is_container. Each one keeps the expression it detected before, so has_to_string still reads through ADL alone, and is_container still lets a string-like type lose to is_stringlike. is_detected and is_detected_v stay. A consumer brings its own expression alias, and krattlink does exactly that in MessageCodec.h. sprint.h had two detection aliases inside string_buffer, and a concept cannot live in a class. Both moved to namespace scope as has_ostream_op and has_member_sbuf_op, above a forward declaration of string_buffer. mutex.h, binary_stream.h and sprint.h took a requires clause each. delegate.h took eight, and its two conditions read as named constants now, not_copy_ctor and args_match, so the positive and the negative constructor pair state the same rule once. Two tests moved with the traits. masked_module_only.cpp writes its own expression alias, which proves the idiom rather than one alias ReCpp used to export. test_modules_identity.cpp lost its anchor, because no variable template is left in type_traits.h, so it takes the address of rpp::PI instead. That constant is one math.h shipped without inline, which is the defect the case guards. test_sprint.cpp gains a case which pins all eleven traits against the expressions they name. Verified: the modules build passes 557/557, the header build passes 534/534, the TSAN build passes 557/557, the module consumer exits 0 on the modules path, all five gates report no finding, and clang-tidy reports nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PJQak2qMQ4cXHtqEcpwimN
RedFox20
changed the title
Five items, and a measurement backs each one. clang-tidy reported performance-enum-size on the scoped enum in test_sprint.cpp, which three jobs failed on. The enum takes uint8_t now. A probe reproduces the warning on the old spelling and reports nothing on the new one. The two clang TSAN jobs failed on the test this branch added. clang links its sanitizer runtime statically, so the linker binds __tsan_default_suppressions and dlsym never sees it. The dlsym probe is a gcc case now, and clang reads the hook directly. C25 stands: the defect and its fix are both gcc-only. The generator dropped a guard configuration on any parse error, so a real error in another header would have produced an unguarded export. NO_CONFIG names the one header which does not compile bare metal, and every other diagnostic reaches the caller. The selftest pins both directions. C25 was several paragraphs, and R9b asks for two sentences. The plan carried two stale counts, 20 remaining headers and six of eight L2 modules, and both read 18 and eight now. Verified: the modules build passes 557/557, the header build passes 534/534, both TSAN builds pass, the module consumer exits 0 on the modules path, clang-tidy reports nothing on C++20, and all five gates report no finding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PJQak2qMQ4cXHtqEcpwimN
clang-tidy on C++23 reported two more findings in the case this branch added. readability-named-parameter wants the ADL probe argument named, and performance-enum-size wants a base type on the unscoped enum too. The enum still promotes to int, so std::to_string takes it and the assertion holds. The C++20 job stayed quiet on both, which is why the last push missed them. A standalone probe reports 2 findings on the old spelling and 0 on the new one. Verified: clang-tidy reports nothing on C++20 and on C++23, the modules build passes 557/557, the header build passes 534/534, and the TSAN build passes 557/557. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PJQak2qMQ4cXHtqEcpwimN
The command AGENTS.md names as the clang-tidy gate exits 0 on a warm build tree and reports no finding, while the CI job of the same name fails. Two rounds on this pull request went red that way, and the commit before this one blamed a C++20 against C++23 difference. That was wrong. The C++20 job reported both findings too. mama reuses the build directory a plain build configured, so CMAKE_CXX_CLANG_TIDY never lands in the cache and no translation unit is analyzed. Adding `configure` sets it, and then a gcc-14 build stops, because clang-tidy cannot parse the gcc module flags. B11 carries the evidence and a standalone command which does check one finding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PJQak2qMQ4cXHtqEcpwimN
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04a55cce89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ducer The module-only consumer names std::declval and included only <string> and <vector>. libstdc++ declares it through <string>, so the gate passed by luck, which is what the include-what-you-use rule exists to stop. B10 told a reader to run the C25 loop, and the R9b cut removed that loop in the same push. B10 carries its own loop now, which greps for heap-use-after-free instead of a race. Verified: the module consumer exits 0 on the modules path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PJQak2qMQ4cXHtqEcpwimN
The concepts commit moved seven delegate declarations from enable_if_t to a requires clause, and three of them had no test which the constraint could break. A mutation over each one says which. Removing not_copy_ctor from the master constructor makes the suite segfault, but only in the case this commit adds. The unconstrained template beats the copy constructor for a non-const lvalue, so a delegate wraps itself and recurses. Every case which ran before that one passed, so the suite never saw it. Swapping the args_match pair on the two const-instance constructors crashes the new const case. Removing one constraint of that pair is benign, because C++20 partial ordering prefers the constrained candidate over an unconstrained one. That is a real difference from the enable_if form, where both were viable. Two cases close a coverage gap rather than pin a constraint. reset() had no test at all, and it takes the same constraint as the master constructor, so the case drives all three dispatch branches plus the two member overloads. operator= had only the delegate side through copy_operator_lambdas, so the callable side takes a case of its own. The const-instance case is the first test to bind a member function through a const object. `methods` and the decay_adapter cases all pass a non-const pointer, which the const overload accepts by conversion. Verified: the modules build passes 561/561, the header build passes 538/538, the TSAN build passes 561/561, clang-tidy reports nothing over the changed file, and all five gates report no finding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PJQak2qMQ4cXHtqEcpwimN
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac6f14578a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The four delegate cases run in both modes, so the handover table reads 561/561 and 538/538 now. The last count refresh landed with the trait cases, one commit before this one added them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PJQak2qMQ4cXHtqEcpwimN
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.
Three independent changes, plus the review round. Two close an open entry in
BUGS.md, and one opens a new one.1. B6:
libtsan.sonever read the suppression hook (181035c)B6 blamed the libc++ spelling of the C15 pattern, and the cause is one attribute. gcc links
libtsan.so, which reads__tsan_default_suppressionsthrough the global dynamic symboltable.
-fvisibility=hiddenkept the definition out of it, sodlsymanswered with the weakhook inside
libtsan.so, which returns null. Every pattern was dead since C15. clang linksits runtime statically, which is why C15 worked there.
The hook and its patterns moved to
tests/test_sanitizers.cpp, beside a test which reads thestring back. On gcc it calls
dlsym(RTLD_DEFAULT, ...), and that call fails without theattribute. clang takes the hook directly, because its static runtime never enters
.dynsym.Both reports on master name
test_future::test_except_handler_chaining, which shares oneexception object between two pool workers. The refcount which orders them sits in an
uninstrumented
libstdc++.so, so TSAN sees no edge and calls the free a race.Measured on 2 pinned cores: 3 reports in 120 runs before, 0 in 120 after. Ten full-suite runs
matched no suppression at all, which is what B6 asked for.
One report survived the suppression, and it is a different defect. A pool worker reads its
semaphore after the pool destroyed it, so
BUGS.mdB10 carries it.2. B8: two shapes gcc-14 cannot write (
7b2db16)B8 said gcc-14 writes a module for
sprint.handtask.hthat no importer reads, and itnamed a compiler defect with no shape. Both modules ship now. A bisect outside cmake, which
builds every
.cppminto onegcm.cacheand answers in seconds, found two shapes.Shape 1 reaches
rpp.sprintthroughrpp.type_traits. An export which names a functionin the
std::__cxx11inline namespace makes the module unreadable.std::to_stringandstd::stoiboth do it, andstd::swapdoes not. The form does not matter. Anis_detected_valias, a plain alias template and a C++20 concept all fail the same way, and so does a concept
which calls an unexported helper that names it. Only dropping the export works.
So the generator gains
NO_EXPORT, andhas_std_to_stringleaves the module surface. Aheader includer still gets the trait.
Shape 2 reaches
rpp.task. A module which includesfuture_types.hin its global modulefragment and also imports
rpp.future_typeswrites an unreadable.gcm. Either half alone isfine, and the importer only fails when it also includes
<rpp/tests.h>.NO_IMPORTdropsthat one re-export.
3. The detection idiom becomes C++20 concepts (
21920c4)ReCpp demands C++20, and
config.hcarries astatic_assertwhich proves it. So every traitbuilt on
is_detected_vbecame a concept, and everyenable_if_tsite became arequiresclause.
src/rppcarries neither spelling now.type_traits.hdrops eight expression aliases and turns seven traits into concepts. Each onekeeps the expression it detected before, so
has_to_stringstill reads through ADL alone, andis_containerstill lets a string-like type lose tois_stringlike.is_detectedandis_detected_vstay, because a consumer brings its own alias, and krattlink does exactly thatin
MessageCodec.h.sprint.hhad two detection aliases insidestring_buffer, and a concept cannot live in aclass. Both moved to namespace scope as
has_ostream_opandhas_member_sbuf_op.mutex.h,binary_stream.handsprint.htook arequiresclause each.delegate.htookeight, and its two conditions read as named constants now.
test_modules_identity.cpplost its anchor, because no variable template is left intype_traits.h, so it takes the address ofrpp::PIinstead. That constant is onemath.hshipped without
inline, which is the defect the case guards.4. The review round, and a gate which was checking nothing (
b59eb6a,f039f52,04a55cc,c9497d9)Codex raised six findings. Five are fixed: the generator now re-raises a parse error the
NO_CONFIGallowlist does not name, C25 is two sentences, the plan counts read 18, themodule-only consumer includes
<utility>, and B10 carries its own reproducer. The sixth asksto remove the test-scoped TSAN pattern, and that thread stays open with the measurement,
because removing it reproduces B6 and no narrower pattern reaches the second report.
Two CI rounds went red on clang-tidy findings in the new test, and chasing them exposed
B11.
CXX20=1 mama gcc build clang-tidy test="nogdb -vv", which AGENTS.md names as thegate, leaves
CMAKE_CXX_CLANG_TIDYunset because mama reuses the directory a plain buildconfigured. It exits 0 having analyzed nothing. Adding
configuresets the variable, and thena gcc-14 build stops, because clang-tidy cannot parse the gcc module flags. The CI gcc-13 jobs
never hit that, because gcc-13 builds no modules.
Verification
clang-tidy-18directlyL2 is complete, so twenty-six modules ship.
docs/MODULES_MIGRATION.mdrevision 10 carriesthe handover state.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PJQak2qMQ4cXHtqEcpwimN