fix: make cobra-core build under MSVC - #50
Open
w00tzenheimer wants to merge 1 commit into
Open
Conversation
Two portability defects surfaced by building on windows-latest: 1. StructureRecovery.cpp used __builtin_popcountll, a GCC/Clang extension with no MSVC equivalent (error C3861, plus four cascading errors as the surrounding reference failed to deduce). Replaced with std::popcount from <bit> -- available since C++20 and the project already requires C++23. 2. /sdl promotes C4146 'unary minus applied to unsigned type' to an error, breaking five sites that deliberately compute two's-complement negation for Expr::Kind::kNeg ((-x) & mask, well-defined for unsigned). Added /wd4146 rather than rewriting the idiom at each site.
|
|
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.
Problem
cobra-coredoes not compile with MSVC. Two independent issues, both surfacedby a build on windows-latest (VS 2022):
1.
__builtin_popcountllis a GCC/Clang extension.lib/core/StructureRecovery.cpp:169calls it directly:Four further errors cascade from it as the surrounding
auto &srcfails todeduce (C2530, C3536, C2446 x2).
2.
/sdlpromotes C4146 to an error.CMakeLists.txt:41enables/sdl, which turns this warning into a hard error:It fires at five sites that deliberately compute two's-complement negation for
Expr::Kind::kNeg, all of the form(-x) & mask:Negation of an unsigned value is well defined, and the masking makes the intent
explicit.
Fix
Use
std::popcountfrom<bit>. It is available since C++20 and theproject already sets
CMAKE_CXX_STANDARD 23, so no feature test is needed.The operand is
uint64_t, making this a direct substitution.Add
/wd4146for MSVC. Suppressing reads better than rewriting the idiom atfive sites: the negation is intentional, and the warning carries little
signal in code that does modular arithmetic throughout. The comment in
CMakeLists.txtrecords why.Verification
Builds clean on Windows (MSVC, VS 2022) where it previously failed.
Rebuilt on macOS arm64 to confirm no behaviour change:
Also green on macOS x86_64 and Linux (manylinux_2_28).
Note
Independent of the change itself, building the dependency superbuild on a
machine without a system LLVM also requires the companion fix in #49 to
dependencies/CMakeLists.txt.