fix(c++): propagate /Zc:preprocessor on MSVC for FORY_STRUCT consumers#3694
Merged
chaokunyang merged 1 commit intoMay 20, 2026
Merged
Conversation
The FORY_STRUCT macro in cpp/fory/meta/field_info.h forwards __VA_ARGS__ through nested macros, which the legacy MSVC preprocessor expands as a single token. .bazelrc already adds /Zc:preprocessor for Bazel Windows builds, but CMake consumers had to remember to add it themselves; PR apache#3078 papered over this in the docs sample. Propagate the flag from fory_meta (the library that owns the macro) via PUBLIC target_compile_options gated on a CXX_COMPILER_ID:MSVC generator expression. The genex keeps clang-cl unaffected (it warns on the flag and is already conforming) and keeps the install-export hermetic for non-MSVC consumers reading the exported target through find_package(fory). Closes apache#3693
chaokunyang
approved these changes
May 20, 2026
truffle-dev
added a commit
to truffle-dev/truffleagent-site
that referenced
this pull request
May 21, 2026
Three new merges since the last refresh: Archon#1730 (always_run node opt-out), Archon#1371 (codex AbortController per retry), and apache/fory#3694 (MSVC /Zc:preprocessor). Apache Fory is a new org on the wall.
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
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?
#3693 reports that
FORY_STRUCTfails to compile under MSVC unless/Zc:preprocessoris set. The macro forwards__VA_ARGS__through nested macros, which the legacy MSVC preprocessor expands as a single token..bazelrc:59-60already sets the flag for Bazel Windows builds; CMake consumers had to remember to add it manually. #3078 papered over the gap in the docs sample, but every downstream CMake project that depends onfory::serialization/fory::row_format/fory::forystill has to opt in by hand.What does this PR do?
target_compile_options(fory_meta PUBLIC $<$<CXX_COMPILER_ID:MSVC>:/Zc:preprocessor>)incpp/fory/meta/CMakeLists.txt.fory_metabecause that target ownsFORY_STRUCTincpp/fory/meta/field_info.h; PUBLIC propagates through the existing link chain (fory_meta->fory_serialization/fory_row_format/fory_encoder-> the user-facing INTERFACE wrappers) without duplicating on three targets.CXX_COMPILER_ID:MSVC(not the legacyif(MSVC)variable) so the flag is omitted forclang-cl(compiler idClang), which warns on/Zc:preprocessorand is already conforming. The genex also keeps the install-export hermetic: a non-MSVC consumer importingfory::metathroughfind_package(fory)reads an empty entry rather than a stuck flag..bazelrc:59-60for Bazel parity.The docs sample at
docs/guide/cpp/index.md:55-57becomes redundant for v0.18.0+ users but is left in place: the sample still pinsGIT_TAG v0.17.0, so removing it would silently break readers following the docs on the prior version.Related issues
Closes #3693
AI Contribution Checklist
yesyes, I included a completed AI Contribution Checklist in this PR description and the requiredAI Usage Disclosure.yes, my PR description includes the requiredai_reviewsummary and screenshot evidence of the final clean AI review results from both fresh reviewers on the current PR diff or current HEAD after the latest code changes.Does this PR introduce any user-facing change?
CMake users on MSVC will automatically receive
/Zc:preprocessorwhen linking any fory target on v0.18.0+. No source-level API or binary protocol change.Benchmark
N/A. Build-config change.
AI Usage Disclosure
cpp/fory/meta/CMakeLists.txt(C++ build system).claude/skills/fory-code-review/SKILL.md; Reviewer B did not. First pass surfaced five actionable items (clang-cl misclassification, install-export hermeticity, scope justification, docs redundancy, MSVC-path verification). Revision moved fromif(MSVC) { target_compile_options(... PUBLIC /Zc:preprocessor) }to the genex form$<$<CXX_COMPILER_ID:MSVC>:/Zc:preprocessor>; rerun on rev2 returned no further actionable findings from either reviewer. Reviewer A explicitly: "NO FURTHER FINDINGS. Diff is clean." Reviewer B: "NO BLOCKING FINDINGS. The change is correct."$<CXX_COMPILER_ID:MSVC>is the modern idiom over the legacyMSVCvariable, PUBLIC propagation through the link chain is correct, install-export carries the genex verbatim and re-evaluates per consumer. Both transcripts available on request.fory_metasucceeds;examples/cpp/hello_world(which usesFORY_STRUCTviafory::serialization) builds and runs end-to-end (./hello_worldserializes/deserializes all 8 sample types).grep -c '/Zc:preprocessor' compile_commands.jsonreturns 0 on Linux, confirming the genex evaluates to empty for non-MSVC. MSVC path is not exercised locally on the Linux VM; the change mirrors.bazelrc:59-60(already in production for Bazel Windows) and PR docs(c++): Add MSVC compatibility to the CMake sample in the CPP document. #3078 (merged docs precedent). A Windows CMake smoke job would be the ideal proof and is a reasonable follow-up..bazelrcsettings.NIT from Reviewer B (not addressed; low priority): adding
MSVC_VERSION GREATER_EQUAL 1925fast-fail to surface the floor explicitly rather than silently warning on older MSVC.FORY_STRUCT's inline comment already documents requiring VS 2022 17.11+ for in-class usage, so older MSVC users will hit unrelated errors regardless; leaving the floor implicit matches the existing.bazelrcprecedent.