Fix missing return statements in SetFunction placeholders (breaks MSVC/Windows builds) - #58
Open
Fkroune wants to merge 1 commit into
Conversation
evaluate(), evaluateWithMemoization(), marginalGain(), marginalGainWithMemoization(), and getEffectiveGroundSet() are pure virtual methods (SetFunction.h) that also carry empty out-of-line definitions here in the base class. Every concrete subclass overrides them with a real implementation, so these bodies are unreachable in practice -- but their non-void return types with no return statement at all is undefined behavior per the C++ standard regardless of reachability, and MSVC's optimizer (whole-program /LTCG) treats it as a hard link-time code generation failure (LNK1257) rather than the warning GCC/Clang emit, blocking any Windows build of this package from source. Fix: each placeholder now returns a default value of its own declared type (0.0 for the double-returning methods, an empty unordered_set for getEffectiveGroundSet) -- semantically inert since these bodies are never actually executed by real call sites, and removes the undefined behavior regardless. Found and being verified via a downstream project's (rushcut, github.com/Fkroune/RushCut) real Windows CI build, which hit this exact LNK1257 failure building submodlib from source.
This branch has not been deployed
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.
What
SetFunction::evaluate,evaluateWithMemoization,marginalGain,marginalGainWithMemoization, andgetEffectiveGroundSet(cpp/SetFunction.cpp) are pure virtual methods (seecpp/SetFunction.h) that also carry empty out-of-line base-class definitions here — every concrete subclass (FacilityLocation,GraphCut, etc.) overrides them with a real implementation, so these particular bodies are never actually executed. But each has a non-voidreturn type with noreturnstatement at all, which is undefined behavior per the C++ standard regardless of reachability.GCC/Clang only warn about this (
-Wreturn-type). MSVC's optimizer, specifically with whole-program optimization (/LTCG, the default for a Releasesetup.pyextension build), treats it as a hard link-time code generation failure:This means
pip install submodlibcurrently cannot build from source on Windows at all (confirmed on a cleanwindows-latestGitHub Actions runner, MSVC from Visual Studio 2026/toolset 14.51).Fix
Each placeholder now returns a default value of its own declared type (
0.0for thedouble-returning methods, an emptyunordered_setforgetEffectiveGroundSet) — semantically inert, since these bodies are never actually called by any real code path, and removes the undefined behavior either way.Minimal, mechanical, 5-line diff — no behavior change for any real usage.
Verification
Found via, and being verified against, a downstream project's real Windows CI build (adding it to this PR once that run completes).