Replace add_axioms_for_is_empty body with length(s) == 0 form#8915
Open
tautschnig wants to merge 1 commit into
Open
Replace add_axioms_for_is_empty body with length(s) == 0 form#8915tautschnig wants to merge 1 commit into
tautschnig wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #8915 +/- ##
========================================
Coverage 80.60% 80.60%
========================================
Files 1711 1711
Lines 189466 189464 -2
Branches 73 73
========================================
+ Hits 152719 152721 +2
+ Misses 36747 36743 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Removes the deprecated add_axioms_for_is_empty helper from the string constraint generator and keeps is_empty support by inlining its logic in the function-application dispatch.
Changes:
- Removed the
add_axioms_for_is_emptydeclaration from the public generator interface. - Deleted the deprecated
add_axioms_for_is_emptyimplementation fromstring_constraint_generator_testing.cpp. - Inlined the
is_emptyconstraint generation directly intoadd_axioms_for_function_application.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/solvers/strings/string_constraint_generator.h |
Drops the deprecated add_axioms_for_is_empty method declaration. |
src/solvers/strings/string_constraint_generator_testing.cpp |
Removes the deprecated method definition and adjusts includes accordingly. |
src/solvers/strings/string_constraint_generator_main.cpp |
Replaces the call to the removed method with equivalent inlined constraint-generation logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
84b72f5 to
e534d61
Compare
The 2017 DEPRECATED tag on string_constraint_generatort:: add_axioms_for_is_empty pointed at a simpler encoding -- "should use string_length s == 0 instead". Adopt that encoding now: drop the auxiliary boolean symbol and the two implications that previously defined `is_empty <=> (length == 0)`, and just return `length(s) == 0` directly (with `typecast_exprt::conditional_cast` to the requested boolean shape, which is a no-op when the call site already requests `bool_typet`). This retains observable behaviour (the SMT-level value of `is_empty(s)` is unchanged) while eliminating one fresh boolean variable and two existential constraints per call site, and matches what the deprecation note recommended eight years ago. The `DEPRECATED` macro on the helper is dropped because the body now is the recommended encoding. Restore `add_axioms_for_is_empty` as a non-deprecated private helper on `string_constraint_generatort` and replace the previously-inlined block in `add_axioms_for_function_application` with a single-line dispatch, matching the surrounding `else if(id == ID_X) return add_axioms_for_X( expr);` pattern of every other branch. Add jbmc/regression/jbmc-strings/java_isempty_nondet/ to actually exercise the new helper under cbmc-CORE. The pre-existing `java_empty` and `ConstantEvaluationIsEmpty` tests use a constant string literal, which the simplifier folds via `simplify_string_is_empty` in `src/util/simplify_expr.cpp` before the call ever reaches the string-refinement loop. The new test passes a nondet `String` argument through `--function ...checkIsEmpty`, which defeats simplification and therefore causes the constraint generator dispatch to actually be hit. With the helper deliberately replaced by `INVARIANT(false, ...)`, the new test triggers the invariant violation during string-refinement, confirming that it does cover the changed code path. Fixes: diffblue#8019 Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
e534d61 to
9f5e2ba
Compare
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.
Adopt the encoding the 2017
\deprecatedtag onstring_constraint_generatort::add_axioms_for_is_emptywas pointing at — "should usestring_length s == 0instead" — and drop the deprecation now that the body matches the recommendation.The previous body introduced a fresh boolean symbol
is_emptyand constrained it via two existentialsis_empty ⇒ |s|=0and|s|=0 ⇒ is_empty, then returned a typecast of that symbol. The two implications together defineis_empty ⇔ (length(s) == 0), so the auxiliary symbol was pure indirection. Replace this with a direct return oflength(s) == 0:Observable behaviour is unchanged (the SMT-level value of
is_empty(s)still denoteslength(s) == 0); the encoding now generates one fewer boolean variable and zero existential constraints per call site. TheDEPRECATEDmacro is removed because the body is no longer the discouraged form.add_axioms_for_is_emptyis restored as a non-deprecated private helper onstring_constraint_generatort, and the previously-inlined block inadd_axioms_for_function_applicationis replaced with a single-line dispatch, matching the surroundingelse if(id == ID_X) return add_axioms_for_X(expr);pattern of every other branch in the chain.Test coverage
A new CORE-tagged regression test,
jbmc/regression/jbmc-strings/java_isempty_nondet/, exercisesadd_axioms_for_is_emptyfor a non-constant string. The pre-existingjava_empty/andConstantEvaluationIsEmpty/tests use literal strings, whichsimplify_string_is_emptyinsrc/util/simplify_expr.cppconstant-folds before the call reaches the string-refinement loop — so they do not cover the helper. The new test passes a nondetStringargument through--function test_isempty_nondet.checkIsEmpty, defeating simplification and forcing the dispatch arm to be taken.Coverage was confirmed by temporarily planting
INVARIANT(false, ...)at the start of the helper: the new test triggers the invariant violation in the string-refinement loop, whilejava_empty/runs to completion (the simplifier folds the call before the helper is reached). With the real implementation restored, both tests pass.Fixes: #8019