Fix Windows build compatibility (MSVC cl.exe and clang-cl)#53
Merged
wavefunction91 merged 51 commits intoJun 12, 2026
Conversation
- Remove dead `exx_coeff` static constexpr in `deorbitalized.hpp` that referenced non-existent members in kernel_traits (MSVC eagerly instantiates it; GCC/Clang deferred since it was never ODR-used) - Replace C++ alternative operator tokens (`not`, `and`, `or`) with standard `!`, `&&`, `||` across headers and source files (MSVC cl.exe does not recognize them without /permissive-) - Add `_USE_MATH_DEFINES` compile definition for MSVC (M_PI) - Suppress MSVC warning C4800 (implicit double-to-bool conversion in auto-generated kernel code) - Replace `sed` PATCH_COMMAND for libxc with portable CMake -P script so FetchContent works on Windows without Unix tools
Co-authored-by: Copilot <copilot@github.com>
warning C4996: 'strdup': The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _strdup.
This was referenced May 7, 2026
There was a problem hiding this comment.
Pull request overview
This PR targets Windows toolchain compatibility (MSVC cl.exe and clang-cl) by removing MSVC-sensitive template instantiation triggers, avoiding alternative operator tokens, and making the libxc FetchContent patch step portable.
Changes:
- Replaced
not/and/ortokens with!/&&/||in several translation units and headers. - Updated CMake to add MSVC/clang-cl-specific definitions and warning suppressions; replaced a
sed-based libxc patch with a CMake-Pscript. - Removed a dead
exx_coeffconstexpr indeorbitalized.hppand adjusted exception code to address MSVC deprecation warnings.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/xc_functional.cxx |
Replaces alternative operator tokens to improve MSVC compatibility. |
src/libxc.cxx |
Replaces or with ` |
src/CMakeLists.txt |
Adds MSVC/clang-cl compile definitions and warning suppression flags. |
src/builtin_interface.cxx |
Replaces or with ` |
include/exchcxx/xc_functional.hpp |
Replaces not/and with !/&& in sanity/type checks. |
include/exchcxx/impl/builtin/kernels/screening_interface.hpp |
Replaces not in if constexpr to satisfy MSVC parsing/compat. |
include/exchcxx/impl/builtin/kernels/deorbitalized.hpp |
Removes a dead constexpr that referenced non-existent traits members. |
include/exchcxx/exceptions/exchcxx_exception.hpp |
Switches to _strdup and updates macro to avoid not. |
CMakeLists.txt |
Replaces a sed PATCH_COMMAND with a portable CMake script invocation. |
cmake/patch_libxc_work_mgga.cmake |
New CMake script to patch libxc sources without Unix tools. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wavefunction91
requested changes
May 26, 2026
This reverts commit 871fcc1.
The previous implementation called _strdup() (MSVC-only; unknown to GCC/Clang on POSIX) on a stringstream result inside what(), which both broke portability and leaked the allocated C string on every call. Build the formatted message once in the constructor and store it as a std::string member; what() now returns its .c_str(). This removes the need for strdup entirely, fixes the leak, and makes what() truly noexcept (no allocation or stringstream work on the noexcept path). Also drop the now-unused <string.h> include and move string parameters into members to avoid an extra copy.
/EHsc enables the standard C++ exception-handling model, eliminating
C4577 ('noexcept' used without exception handling mode specified).
/Zc:preprocessor enables MSVC's conforming preprocessor (ISO C11/C17
behaviour), which correctly treats an empty macro argument as a valid
single argument. This silences C4003 ('not enough arguments for
function-like macro invocation') that arose wherever NOTYPE — defined
as an empty token — was passed to TYPED_*_OPARAMS_*() macros in the
FORWARD_XC_ARGS expansion chain.
…eaders The ~44 000 C4800 occurrences are all in auto-generated XC kernel implementation headers under include/exchcxx/impl/builtin/kernels/. These files are produced by a code-generation script and are not hand-maintained; patching them individually is not sustainable. The conversion is intentional (the generated code encodes boolean screening conditions as doubles that happen to carry 0.0/non-zero values), so the warning carries no actionable signal here.
C4514 fires when the compiler removes an inline function that was never called in a translation unit. The warning is purely informational (level 4, off by default): the linker dead-strips these symbols correctly and there is no correctness risk. In ExchCXX the 1 097 occurrences come from the XcKernel helper overloads in xc_kernel.hpp that are specialised per kernel type but not needed in every TU.
C4710 ('function not inlined') and C4711 ('function selected for
automatic inline expansion') are level-4 informational diagnostics that
report the compiler's inlining decisions. They carry no correctness
signal; whether a function gets inlined is a QoI choice left to the
optimizer. The ~1 400 combined occurrences all originate from STL
string/exception machinery pulled in by exchcxx_exception.hpp and
screening_interface.hpp.
C4061 fires for every Kernel enumerator that falls through to a default: label in kernels.hpp. The switch is intentionally written with a catch-all default that throws an exception for unrecognised kernels; every new enumerator is handled uniformly by that path. Requiring an explicit case for each of the 100+ kernel variants would make the switch unmaintainable without adding safety.
C4266 fires on BuiltinKernelImpl because the base class BuiltinKernel declares pure-virtual eval_exc/eval_fxc/... overloads for all three approximation types (LDA, GGA, MGGA) and each specialisation only overrides its own subset, intentionally inheriting the base not-implemented default for the other types. MSVC treats the unoverridden virtuals as a warning even though that design is correct.
The 160 C4702 occurrences are in builtin_kernel.cxx in the large kernel-name-to-type dispatch switch. After each case the function returns; MSVC considers any following statement unreachable. These are artefacts of macro-expanded dispatch tables where the compiler loses track of control flow, not real dead code. Suppressing avoids noise without hiding genuine unreachable-code bugs elsewhere (none present).
The 60 C4365 occurrences are in xc_functional.cxx and libxc.cxx where int loop indices are compared against or passed to STL container methods that take size_type (size_t). C4365 is a level-4 diagnostic not in the mandatory warning list. The mismatches do not represent real bugs: the loop bounds are always non-negative and well within size_t range. Fixing them would require pervasive signed/unsigned casts that reduce readability for no safety gain.
C5045 is an informational remark (not a true warning) telling the user that the compiler would insert a Spectre v1 load fence if /Qspectre were passed. ExchCXX is a numerical library, not security-sensitive code, and /Qspectre is not enabled in this build. The remark adds no actionable information.
C4820 ('bytes of padding added after data member') is a level-4
diagnostic about ABI layout decisions made by the compiler; callers
cannot rely on internal struct layout.
C4626 ('assignment operator was implicitly defined as deleted') fires
because BuiltinKernelInterface and LibxcKernelInterface hold reference
members, making copy-assignment impossible by design. The classes are
intended to be non-copyable; the deleted operator is correct behaviour.
Both are purely informational and not in the mandatory warning list.
…d.hpp In Deorbitalized<XCEF,KEDF> the deorbitalization substitutes tau with ke_traits::eval_*_unpolar/polar called with tau=0.0, so the incoming tau/tau_a/tau_b parameters from the MGGA interface are never read. Mark them [[maybe_unused]] to satisfy C4100. vtau_k was declared alongside the other kinetic-energy VXC outputs but all tau outputs were already captured through the dummy variable in the ke_traits calls; vtau_k was never written to or read. Remove it to fix C4101 (local variable initialized but not referenced).
The buffer_len() helpers on XCKernelImpl return size_t; storing the
results in int locals caused C4267 ('conversion from size_t to int,
possible loss of data'). Changed the len_* variables in
xc_kernel_test.cxx to size_t, which also matches the vector<> size_type
they are used with.
In reference_values.cxx the load_reference_*() helpers declare int npts
and assign from std::vector::size() (size_t). Added explicit
static_cast<int> at each assignment; npts stays int because the
functions return std::pair<int,...> and the values are always small
enough to fit.
In xc_functional_test.cxx npts was declared size_t but passed directly
to eval_exc_vxc() whose first parameter is int N; changed to int.
These changes also remove the C4267 cascade into xc_kernel.hpp's
template forwarding wrappers, which fired because those templates were
instantiated with size_t as the N argument.
C4018 ('signed/unsigned mismatch'): the for-loop upper bounds were the
len_* buffer-length variables, which were previously int; comparing them
against the unsigned loop index (0ul) triggered C4018. The previous
commit already changed those variables to size_t, so no loop-body
changes are needed here.
C4701 ('potentially uninitialized variable'): 'backend' was declared
without an initializer and only set inside nested Catch2 SECTION blocks.
MSVC cannot prove it is always assigned before use because the SECTION
macro expands to control-flow that is opaque to the analyser. Giving
'backend' a default value of Backend::builtin at its declaration
satisfies C4701 without changing observable test behaviour (each SECTION
that cares overrides it explicitly).
xcNumber was declared before a #if XC_MAJOR_VERSION > 7 block that was
the only place it was referenced. When building against libxc 7 or
earlier the variable is initialised but never read, triggering C4189
('local variable is initialized but not referenced').
Move the declaration inside the #if guard and change the spanning
return-expression pattern to explicit return statements on each branch,
which also makes the preprocessor intent clearer.
… eval site Previous fix changed npts from size_t to int, which was correct for MSVC but introduced an implicit int→size_t conversion at the buffer_len call sites that GCC/Clang would warn about with -Wsign-conversion. Better approach: keep npts as size_t (correct type for buffer lengths) and add static_cast<int> only at the eval_exc_vxc call sites where int N is required. No conversion warnings on any compiler.
Under clang-cl, -Wall maps to /Wall (= -Weverything), flooding the build with ~160k C++98-compat and other noise warnings. Policy-required flags are already set explicitly in the if(MSVC) block; the GCC/Clang developer set (-Wall -Wextra -Wpedantic -Wnon-virtual-dtor -Wshadow) must not apply.
libxc/src/functionals.c and version.c use strcpy/sscanf which MSVC and clang-cl flag as deprecated (-Wdeprecated-declarations / C4996). Add _CRT_SECURE_NO_WARNINGS as a PRIVATE compile definition on the xc target rather than touching upstream libxc sources.
xc_kernel_test.cxx uses 'not' (ISO C++ alternative token for !). MSVC cl does not recognise it without conformance mode; /permissive- enables it alongside /EHsc in the top-level add_compile_options block.
Contributor
Author
|
Build tested with both MSVC -DCMAKE_CXX_FLAGS="/W3 /w14018 /w14055 /w14100 /w14102 /w14127 /w14146 /w14242 /w14244 /w14245 /w14254 /w14267 /w14302 /w14306 /w14308 /w14310 /w14389 /w14509 /w14510 /w14512 /w14532 /w14533 /w14610 /w14611 /w14700 /w14701 /w14703 /w14789 /w14995 /w14996" `
-DCMAKE_CXX_FLAGS_RELEASE="/O2 /DNDEBUG" `+ same for C flags |
buffer_len functions take and return size_t; using int caused an implicit narrowing conversion on the argument and a signed/unsigned mismatch in the equality checks.
add_compile_options without a language guard applies flags to all languages including C (libxc sources). Use COMPILE_LANGUAGE:CXX generator expressions so the C++-only flags don't affect C compilation.
/wd* flags and -Wno-* are build-hygiene settings for ExchCXX's own sources and must not propagate to downstream consumers via PUBLIC. /Zc:preprocessor stays PUBLIC because the public headers use the NOTYPE empty macro which requires the conforming preprocessor on consumers too. _USE_MATH_DEFINES stays PUBLIC for the same reason (M_PI in public headers).
Move /EHsc and /permissive- from add_compile_options in the top-level CMakeLists.txt to target_compile_options on the exchcxx target in src/CMakeLists.txt, where they belong. /EHsc applies to both cl and clang-cl; /permissive- and /Zc:preprocessor remain cl-only. Remove all /wd* warning suppressions — the ExchCXX source builds cleanly at default warning level without them. Apply /EHsc to the fetched Catch2 and Catch2WithMain targets and to catch2_main to silence C4530 from MSVC STL headers compiled as part of the Catch2 library.
- exchcxx_exception.hpp: add missing #include <utility> for std::move - src/CMakeLists.txt: move /permissive- from PUBLIC to PRIVATE - test/xc_functional_test.cxx: centralize static_cast<int>(npts) into a single npts_i variable at the call boundary
… C4701, C4703) These warnings are intrinsic to libxc's C code and do not indicate issues in how ExchCXX calls libxc functions.
C4101: unreferenced local variable in hyb_gga_x_cam_s12.c C4996: sprintf deprecation in xc-threshold.c — _CRT_SECURE_NO_WARNINGS alone is insufficient when the consumer sets /w14996
Add _CRT_SECURE_NO_WARNINGS to xc-threshold, and remove redundant /wd4996 from the xc target since the define already prevents CRT deprecation annotations from being applied.
backend is only used inside the #ifdef EXCHCXX_ENABLE_LIBXC block; when libxc is disabled the parameter is unused, triggering clang-cl -Wunused-parameter.
wavefunction91
requested changes
Jun 11, 2026
wavefunction91
left a comment
wavefunction91
left a comment
Owner
There was a problem hiding this comment.
Only one minor comment, then this goes in.
wavefunction91
approved these changes
Jun 12, 2026
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.
Summary
Enable ExchCXX to build and test cleanly with both MSVC
cl.exeandclang-clon Windows.Compiler compatibility fixes
exx_coeffstatic constexpr indeorbitalized.hpp(MSVC eagerly instantiates; GCC/Clang defer since never ODR-used)not,and,or) with!,&&,||(MSVC requires/permissive-otherwise)_USE_MATH_DEFINEScompile definition forM_PIsedPATCH_COMMAND for libxc with portable CMake-Pscript so FetchContent works on WindowsBuild system
/EHsc(C++ exceptions) and/permissive-(alternative tokens) scoped to C++ targets/Zc:preprocessor(conforming preprocessor) PUBLIC — required by consumers for NOTYPE empty macroCMakeLists.txtas part of the project build definition-Wall -Wextra -Wpedantic) behindif(NOT MSVC)/wd*and-Wno-*flags PRIVATE (not propagated to consumers)/EHscto FetchContent'd Catch2/Catch2WithMain targetsMSVC warning fixes (all fixed at source)
tauparams andvtau_kvariable indeorbitalized.hppxcNumberunused when building against libxc ≤ 7size_t→intconversions in test files — use correct types and explicit casts ateval_*call boundariesxc_kernel_test.cxxException refactor
strdup/_strdupwith storedstd::stringmember inexchcxx_exception::what()— fixes POSIX portability and memory leakLibxc FetchContent warning suppression
Since libxc is fetched via FetchContent, its warnings cannot be fixed upstream here. Suppress them per-target:
_CRT_SECURE_NO_WARNINGSon bothxcandxc-thresholdtargets-Wno-logical-op-parentheses,-Wno-sometimes-uninitialized,-Wno-uninitialized,-Wno-unused-but-set-variable,-Wno-unused-const-variable,-Wno-unused-function,-Wno-unused-variable/wd4100,/wd4101,/wd4146,/wd4244,/wd4267,/wd4701,/wd4703