refactor(deco,zest): make macro.h a pure macro header in both libraries - #181
Merged
Conversation
…macro.h Modules cannot export macros, so once kotatsu is consumed as a module the Deco* declaration macros stop being reachable through `import`. They have to come from a textual include — but the old home, kota/deco/facade/macro.h, pulled in facade/decl.h and facade/trait.h, which would duplicate the very declarations the module already provides. Move the macros up to kota/deco/macro.h, mirroring the kota/zest/macro.h layout, and drop the deco includes. The expansions only need those names at the point of use, which either kota/deco/deco.h or the imported module supplies. The std headers the expansions name (<cstdint>, <string>, <type_traits>, <vector>) are kept so the header stands on its own next to a module import. Downstreams that included kota/deco/facade/macro.h directly must switch to kota/deco/macro.h; kota/deco/deco.h consumers are unaffected. kota::option has no macros of its own, so it needs no equivalent header.
Contributor
📝 WalkthroughWalkthroughThe deco macro header no longer includes internal declaration headers, adds required standard-library includes, and is tested independently. The build publishes the header, while facade include ordering is adjusted. ChangesDeco macro contract
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/deco/macro_standalone.cc`:
- Around line 1-16: Make the standalone guard translation unit include only
kota/deco/macro.h before validating the Deco* macro definitions, removing
<string>, <vector>, and kota/deco/deco.h from that guard path. Move macro
expansion/parser behavior into a separate compile-only test that explicitly
includes kota/deco/deco.h.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aa339b30-2422-4f5b-a884-c4e8ada8262f
📒 Files selected for processing (4)
include/kota/deco/deco.hinclude/kota/deco/macro.htests/unit/deco/macro_standalone.ccxmake.lua
Same rationale as the deco split, applied to the header that already had the layout: a macro header only earns its keep in a module world if it includes nothing at all. kota/zest/macro.h still pulled in check.h, trace.h, suite.h and snapshot.h, so a downstream that did `import kota;` alongside it would get the module's declarations textually duplicated. Move those four to kota/zest/zest.h, which is where the ~90 test TUs already point. The conditional codec/json include moves there too; macro.h keeps the __has_include gate so the snapshot-JSON macros are still defined exactly when they were, it just no longer performs the include itself. The three TUs that included kota/zest/macro.h without kota/zest/zest.h now include the latter. Both macro headers are verified include-free, and a zest counterpart to the deco guard test locks the contract in.
16bit-ykiko
changed the title
![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: 8b39f0e01b
ℹ️ 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".
__has_include("kota/codec/json/json.h") answered the wrong question. That file
ships with kotatsu, so it is on disk whether or not the JSON backend was
actually built — the gate opened unconditionally and then pulled in a header
that hard-errors with "simdjson.h is required for the JSON codec backend".
KOTA_CODEC_ENABLE_SIMDJSON=OFF therefore could not build the test suite at all,
since tests/unit/zest is always compiled. And under modules the probe is
meaningless anyway: the consumer imports the codec rather than including it,
so file existence says nothing about availability.
Drop the gate. The snapshot-JSON macros are now defined unconditionally, like
every other macro in the header — they name ::kota::codec::json, and as with
::kota::meta::eq in EXPECT_EQ, that name only has to be visible where the macro
is used. Using them without the codec is a name-lookup error at the use site,
which is the honest failure.
zest.h no longer includes the JSON codec either; zest does not link it, so
auto-supplying it on the strength of a file-existence probe was the bug. The
one suite that uses these macros now includes the codec itself, behind a
KOTA_TEST_HAS_JSON define that the build systems set when simdjson is enabled —
an actual answer to "is the JSON codec available" rather than a guess.
Verified both ways: with simdjson 943 tests pass (unchanged), and without it
the suite now configures, builds and runs 410 tests where it previously failed
to compile.
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.
Why
Upstream is modularizing, and kotatsu will be built as a module. Modules cannot export macros, so once a downstream switches to
import, theDeco*/TEST_*macros are no longer reachable — they have to arrive through a textual include.For that include to be usable it must pull in nothing. Anything a
macro.hincludes lands in the consumer's TU textually and duplicates declarations the module already provides. Neither library met that bar:kota/deco/facade/macro.hincludedfacade/decl.handfacade/trait.h.kota/zest/macro.hincludedassert/check.h,assert/trace.h,runner/suite.h,snapshot/snapshot.h, plus a conditionalcodec/json/json.h.What
deco — move the macros up to
kota/deco/macro.h, mirroring thekota/zest/macro.hlayout, with zero includes.zest — strip the includes from
kota/zest/macro.h; they move tokota/zest/zest.h, where ~90 test TUs already point.Both headers are now verifiably include-free:
Downstream usage:
Bug found and fixed along the way
kota/zest/macro.hgated theEXPECT_SNAPSHOT_JSONfamily on__has_include("kota/codec/json/json.h"). That answered the wrong question. The file ships with kotatsu, so it is on disk whether or not the JSON backend was built — the gate opened unconditionally and then included a header that hard-errors:Since
tests/unit/zestis always compiled,KOTA_CODEC_ENABLE_SIMDJSON=OFFcould not build the test suite at all onmain. Under modules the probe is meaningless anyway: the consumer imports the codec rather than including it, so file existence says nothing about availability.The fix drops the gate. The snapshot-JSON macros are now defined unconditionally, like every other macro in the header — they name
::kota::codec::json, and just asEXPECT_EQnames::kota::meta::eq, that only has to be visible at the use site.zest.hno longer includes the JSON codec either (zest does not link it). The one suite that uses these macros includes the codec itself, behind aKOTA_TEST_HAS_JSONdefine that both build systems set when simdjson is enabled — an actual answer to "is the JSON codec available" instead of a guess.Breaking changes
kota/deco/facade/macro.hmust switch tokota/deco/macro.h.kota/deco/deco.hconsumers are unaffected; in-repo,deco.hwas the only includer.kota/zest/macro.hand relied on it to supply the zest declarations must includekota/zest/zest.h. Only 3 in-repo TUs were in that position (the other 5 already included both); all 3 are updated here.EXPECT_SNAPSHOT_JSONand relying on zest to auto-include the JSON codec must now includekota/codec/json/json.hitself.Notes
kota::option(include/kota/deco/option/) defines no macros at all — nothing to split there.Deco*ResultErrStringmacros stay infacade/trait.hon purpose: they arestatic_assertmessage strings consumed byfacade/decl.hinside the library, never by a downstream.kota/support/expected_try.hwas already macro-only and include-free — no change needed.#ifdef __cpp_exceptionsgate inzest/macro.hstays: unlike__has_include, that is a genuine property of the TU being compiled.Testing
kota/zest/macro.hdirectly was compile-checked, including the http ones outside the local build config.macro.halone,deco.halone, andmacro.hbeforedeco.h.tests/unit/deco/macro_standalone.ccandtests/unit/zest/macro_standalone_tests.cpp— each include theirmacro.has the very first header,#errorif the public macro set is not defined by that header alone, then exercise the macros with the declarations pulled in afterwards.Possible follow-up (not in this PR)
KOTA_CODEC_TRYlives inkota/codec/visit/context.h, which has 8 includes. If codec is consumed as a module it will need the same treatment.