Fix Windows build compatibility (MSVC cl.exe and clang-cl)#53
Open
lorisercole wants to merge 39 commits into
Open
Fix Windows build compatibility (MSVC cl.exe and clang-cl)#53lorisercole wants to merge 39 commits into
lorisercole wants to merge 39 commits into
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.
…MakeLists.txt The /EHsc, /Zc:preprocessor and /wd* flags were sitting in the PowerShell build helper, which means they were silently absent for any cmake invocation that didn't go through that script (CI, IDEs, sub- project builds, etc.). Move them into the existing if(MSVC) / else() # cl block in src/CMakeLists.txt so they are part of the project's build definition and applied unconditionally whenever cl.exe is the compiler. The commented-out placeholder block that was already there is now replaced with the actual flags plus a rationale comment for each suppression. The build script retains only /W3 and the mandatory /w1XXXX enable flags, which are intentionally kept there as build-policy enforcement rather than project defaults. Note: most /wd* flags are level-4 warnings that become visible under /W3 because MSVC's STL headers internally push the warning level to 4 via #pragma warning(push, 4), causing user template instantiations inside those headers to inherit the elevated level.
…are_libxc_builtin kern.eval_exc / kern.eval_exc_vxc calls in xc_functional_test.cxx were missed in the previous C4267 pass (only func.eval_exc_vxc was fixed). Also cast npts_lda→int at its initialisation site in xc_kernel_test.cxx.
…keLists.txt /EHsc was only on the exchcxx target's PUBLIC options; Catch2 is a separate CMake target compiled without it, triggering C4530 in <chrono> and ppltasks.h during STL template instantiation. Using add_compile_options in the top-level CMakeLists.txt applies it globally to all targets (exchcxx, libxc, Catch2 via FetchContent).
…f_vals.npts ref_vals.npts is int; storing it in size_t then passing it back to eval_* (which takes int) is a pointless round-trip that triggers C4267.
Comment out all clang-cl -Wno-* suppressions that have no /wd* counterpart in the cl build; keep only -Wno-sign-conversion and -Wno-switch-enum which mirror /wd4365 and /wd4061 respectively. Add -Wall -Wextra as the clang-cl policy baseline (equivalent to the /W3 + mandatory /w1XXXX list used for cl); remove /W3 from CMAKE_CXX_FLAGS_RELEASE where it was misplaced.
The MSVC guard previously excluded clang-cl (CMAKE_CXX_COMPILER_ID=Clang), so exceptions were disabled and every 'throw' site failed to compile.
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.
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" `Only a few unnecessarily noisy level-4 warnings have been silenced. |
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).
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.
exx_coeffstatic constexpr indeorbitalized.hppthat referenced non-existentkernel_traitsmembers (MSVC eagerly instantiates; GCC/Clang defer since never ODR-used)not,and,or) with!,&&,||across headers and source files (MSVC cl.exe requires/permissive-otherwise)_USE_MATH_DEFINEScompile definition forM_PIsedPATCH_COMMAND for libxc with portable CMake-Pscript so FetchContent works on Windows without Unix toolsstrdupwith_strdupto fix MSVC C4996 warning