fix(dialog): make PJ_DIALOG_PLUGIN arg-count dispatch MSVC-legacy-preprocessor-safe#153
Merged
Merged
Conversation
…processor-safe The two-arg PJ_DIALOG_PLUGIN(Class, kManifest) form mis-expanded under MSVC's traditional preprocessor (the default without /Zc:preprocessor): __VA_ARGS__ forwarded into PJ_DIALOG_PLUGIN_SELECT arrives as a single glued argument, so the dispatch picks the one-arg LEGACY branch with "Class, kManifest" as ClassName (C2064/C2912 at the call site). Found via pj-official-plugins#230's Windows CI. - Add the canonical PJ_DIALOG_PLUGIN_EXPAND rescan around the selected call, making the macro expand identically under both MSVC preprocessors and GCC/Clang. Header-only; no ABI or call-site change. - Drop the INTERFACE /Zc:preprocessor injection from pj_dialog_sdk: it is no longer needed, imposed a preprocessor mode on consumers, and never reached Conan consumers anyway (CMakeDeps regenerates targets from package_info() and drops upstream INTERFACE_COMPILE_OPTIONS — which is exactly how this shipped unnoticed). - Add mock_dialog_legacy_pp_plugin, a compile-only regression target built with /Zc:preprocessor- on MSVC, so Windows CI now exercises the legacy-preprocessor path the conformant-only SDK build never covered. - CHANGELOG: recorded under the in-flight 0.18.0 (no version bump — folds into the upcoming release). Validated: ./build.sh --debug && ./test.sh — 49/49 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
The bug
The SDK's recommended two-arg
PJ_DIALOG_PLUGIN(Class, kManifest)form mis-expands under MSVC's traditional preprocessor (the default —/Zc:preprocessoroff):__VA_ARGS__forwarded intoPJ_DIALOG_PLUGIN_SELECTarrives as a single glued argument, so the arity dispatch picks the one-argLEGACYbranch with"Class, kManifest"asClassName→C2064(new Class, kManifest()) andC2912(dialogVtableFor<Class, &kManifest>) at every call site. Found by pj-official-plugins#230's Windows CI.Why three layers of protection all missed it:
/Zc:preprocessor(inPJ_WARNING_FLAGS), so the legacy-PP path of the macro was never compiled anywhere.pj_dialog_sdkcarried anINTERFACE /Zc:preprocessorflag for CMake consumers — but Conan's CMakeDeps regenerates targets frompackage_info()and drops upstreamINTERFACE_COMPILE_OPTIONS, so Conan consumers (the official plugins) never received it.The fix
PJ_DIALOG_PLUGIN_EXPANDrescan around the selected call — the canonical MSVC workaround. The macro now expands identically under both MSVC preprocessors and GCC/Clang. Header-only; no ABI change, no call-site change; the existing docs' two-arg examples become true on Windows.INTERFACE /Zc:preprocessorinjection frompj_dialog_sdk— no installed header requires it anymore (this was the only__VA_ARGS__macro in the installed tree), and silently imposing a preprocessor mode on consumers was ineffective for the consumers that needed it. Consumer-visible: CMake consumers' TUs that include dialog headers will no longer be forced into conformant-PP mode — flag it if you'd rather keep it.mock_dialog_legacy_pp_plugin— compiles the mock dialog with/Zc:preprocessor-on MSVC, sowindows-cinow exercises the exact path that broke (no-op extra compile on other platforms).Versioning
Recorded in the CHANGELOG under the in-flight 0.18.0 — no version bump in this PR per Davide.
abi/baseline.abiuntouched (header macro + build-system only).Validation
./build.sh --debug && ./test.sh— 49/49 tests pass.windows-cion this PR).🤖 Generated with Claude Code