This is an automated email from the ASF dual-hosted git repository.
chaokunyang pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/fory.git
The following commit(s) were added to refs/heads/main by this push:
new 0a96e1c29 fix(c++): propagate /Zc:preprocessor on MSVC for FORY_STRUCT
consumers (#3694)
0a96e1c29 is described below
commit 0a96e1c29a94eb8a006fc89a8351f212bcb6380d
Author: Truffle <[email protected]>
AuthorDate: Wed May 20 07:09:16 2026 -0400
fix(c++): propagate /Zc:preprocessor on MSVC for FORY_STRUCT consumers
(#3694)
## Why?
#3693 reports that `FORY_STRUCT` fails to compile under MSVC unless
`/Zc:preprocessor` is set. The macro forwards `__VA_ARGS__` through
nested macros, which the legacy MSVC preprocessor expands as a single
token. `.bazelrc:59-60` already 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 on `fory::serialization` / `fory::row_format` / `fory::fory`
still has to opt in by hand.
## What does this PR do?
- Adds `target_compile_options(fory_meta PUBLIC
$<$<CXX_COMPILER_ID:MSVC>:/Zc:preprocessor>)` in
`cpp/fory/meta/CMakeLists.txt`.
- Placed on `fory_meta` because that target owns `FORY_STRUCT` in
`cpp/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.
- Uses a generator expression on `CXX_COMPILER_ID:MSVC` (not the legacy
`if(MSVC)` variable) so the flag is omitted for `clang-cl` (compiler id
`Clang`), which warns on `/Zc:preprocessor` and is already conforming.
The genex also keeps the install-export hermetic: a non-MSVC consumer
importing `fory::meta` through `find_package(fory)` reads an empty entry
rather than a stuck flag.
- The flag mirrors `.bazelrc:59-60` for Bazel parity.
The docs sample at `docs/guide/cpp/index.md:55-57` becomes redundant for
v0.18.0+ users but is left in place: the sample still pins `GIT_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
- [x] Substantial AI assistance was used in this PR: `yes`
- [x] If `yes`, I included a completed [AI Contribution
Checklist](https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs)
in this PR description and the required `AI Usage Disclosure`.
- [x] If `yes`, my PR description includes the required `ai_review`
summary 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?
- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?
CMake users on MSVC will automatically receive `/Zc:preprocessor` when
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
- **substantial_ai_assistance**: yes
- **scope**: code drafting (single-file CMake change), tradeoff
exploration, PR text drafting
- **affected_files_or_subsystems**: `cpp/fory/meta/CMakeLists.txt` (C++
build system)
- **ai_review**: Two-reviewer loop completed per AI_POLICY.md \xa75.
Reviewer A used `.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 from `if(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.\"
- **ai_review_artifacts**: Both review sessions ran in dedicated
subagents on the rev2 diff. Reviewer A traced each prior finding through
the revision (RESOLVED 1-5) plus left a residual non-blocking note
recommending an MSVC CI smoke job. Reviewer B confirmed
`$<CXX_COMPILER_ID:MSVC>` is the modern idiom over the legacy `MSVC`
variable, PUBLIC propagation through the link chain is correct,
install-export carries the genex verbatim and re-evaluates per consumer.
Both transcripts available on request.
- **human_verification**: Linux/GCC build of `fory_meta` succeeds;
`examples/cpp/hello_world` (which uses `FORY_STRUCT` via
`fory::serialization`) builds and runs end-to-end (`./hello_world`
serializes/deserializes all 8 sample types). `grep -c '/Zc:preprocessor'
compile_commands.json` returns 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 #3078 (merged docs precedent). A Windows CMake
smoke job would be the ideal proof and is a reasonable follow-up.
- **performance_verification**: N/A.
- **provenance_license_confirmation**: Apache-2.0-compatible. No
third-party code introduced. The line in question is original and
analogous to existing in-tree `.bazelrc` settings.
NIT from Reviewer B (not addressed; low priority): adding `MSVC_VERSION
GREATER_EQUAL 1925` fast-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 `.bazelrc` precedent.
---
cpp/fory/meta/CMakeLists.txt | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/cpp/fory/meta/CMakeLists.txt b/cpp/fory/meta/CMakeLists.txt
index 096978240..d75c727d4 100644
--- a/cpp/fory/meta/CMakeLists.txt
+++ b/cpp/fory/meta/CMakeLists.txt
@@ -32,6 +32,18 @@ add_library(fory_meta ${FORY_META_SOURCES})
add_library(fory::meta ALIAS fory_meta)
fory_configure_target(fory_meta)
+# FORY_STRUCT forwards __VA_ARGS__ through nested macros, which the legacy
+# MSVC preprocessor expands as a single token. /Zc:preprocessor selects the
+# standard-conforming preprocessor (MSVC 19.25+ / VS 2019 16.5+) and is
+# propagated to anything that links fory_meta so CMake consumers that use
+# FORY_STRUCT do not need to opt in by hand. The genex gates on the MSVC
+# compiler id (not the legacy MSVC variable) so the flag is omitted for
+# clang-cl, which warns on `/Zc:preprocessor` and is already conforming;
+# the gate also keeps the install-export hermetic for non-MSVC consumers.
+# Mirrors .bazelrc:59-60, which already sets this for Bazel Windows builds.
+target_compile_options(fory_meta PUBLIC
+ $<$<CXX_COMPILER_ID:MSVC>:/Zc:preprocessor>)
+
target_include_directories(fory_meta
PUBLIC
$<BUILD_INTERFACE:${FORY_CPP_ROOT}>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
