Change the return type of constraint_slack
to int64_t
instead of uint64_t
to match the m_slack
member of the constraint
struct
#5360
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.
Brief summary:
In
src/sat/sat_local_search.h
, the return type ofconstraint_slack
should beint64_t
instead ofuint64_t
, as them_slack
member of theconstraint
struct has typeint64_t
. This change currently does not affect behavior for reasons explained below but may prevent the introduction of bugs in the future.Background:
This issue affects the
local_search
class of the sat solver:src/sat/sat_local_search.*
Commit 5e4276b fixes issue #4197 by making a local variable holding the return value of
constraint_slack
signed but fails to correctly adjust the return type ofconstraint_slack
. Fortunately, the (implicit) conversion fromuint64_t
toint64_t
introduced in that commit prevents the incorrect return type ofconstraint_slack
from having an effect. However, it is possible to imagine new code being introduced that callsconstraint_slack
and takes the returned type at face value, failing to convert to a signed type.Details:
In the class
local_search
, we find theconstraint_slack
private method:It looks up the
ci
th constraint in them_constraints
vector and returns the constraint'sm_slack
value. Them_slack
member of theconstraint
struct has typeint64_t
:Moreover,
constraint_slack
is apparently called only twice, both times from withinlocal_search::pick_flip_walksat()
:In each case, a conversion from the
uint64_t
value returned byconstraint_slack
to anint64_t
is performed. (Clang-Tidy points out that such conversion is implementation defined, though in practice I don't think there is an issue here.) Asconstraint_slack
is a private method oflocal_search
, no API or client code should be affected.The "fix" is to change the return type of
constraint_slack
toint64_t
.Related Observations:
pb_coeff
s, which themselves are unsigned, but I do not understand the algorithm enough to know what slack does.int64_t
/uint64_t
vs.unsigned
/int
/long long
/etc. does not appear to be consistent. (Note thatint
andint64_t
can be different sizes.) For example, we have:(It's not clear to me if "slack score" and "slack" have the same units/dimension.)