truffle-dev opened a new pull request, #3694: URL: https://github.com/apache/fory/pull/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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
