Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix configuration generation #126

Merged
merged 11 commits into from
Nov 17, 2023
Merged

Fix configuration generation #126

merged 11 commits into from
Nov 17, 2023

Conversation

danjujan
Copy link
Contributor

@danjujan danjujan commented Oct 6, 2023

@vulder
Copy link
Contributor

vulder commented Oct 6, 2023

@ChristianKaltenecker Do you have an idea what is wrong with the solver here?

For some reason, this caused other configs to appear multiple times during enumeration.
This caused 2 issues:
- either first config was skipped or getCurrentConfig would always point to the next configuration
- getAllValidConfigurations or getNumberValidConfigurations produce incorrect results if called after any call to getNextConfiguration()
The last point is an issue with the Solver API and this is a quick-fix for the Z3Solver implementation that returns an error in that case.
@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (cd3c726) 89.67% compared to head (709341e) 89.63%.

Files Patch % Lines
lib/Solver/Z3Solver.cpp 90.00% 2 Missing ⚠️
include/vara/Solver/ConfigurationFactory.h 83.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           vara-dev     #126      +/-   ##
============================================
- Coverage     89.67%   89.63%   -0.05%     
============================================
  Files            73       73              
  Lines          6991     6982       -9     
============================================
- Hits           6269     6258      -11     
- Misses          722      724       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/Solver/Z3Solver.cpp Outdated Show resolved Hide resolved
lib/Solver/Z3Solver.cpp Outdated Show resolved Hide resolved
lib/Solver/Z3Solver.cpp Outdated Show resolved Hide resolved
This implementation is quite hacky and requires a redesign of the Solver API to be fixed properly.
@boehmseb
Copy link
Member

This issue is more complicated than assumed. The current state of this PR should be considered a quick-fix. Do not merge until it is properly resolved.

That means that model enumeration is no longer part of the solver interface, but should be done using the ConfigurationFactory and ConfigurationIterator.
@boehmseb boehmseb requested a review from vulder November 8, 2023 17:10
@boehmseb
Copy link
Member

boehmseb commented Nov 8, 2023

This is now ready for review.

unittests/Solver/Z3Tests.cpp Outdated Show resolved Hide resolved
unittests/Solver/Z3Tests.cpp Show resolved Hide resolved
@vulder vulder self-requested a review November 17, 2023 09:51
@vulder
Copy link
Contributor

vulder commented Nov 17, 2023

@vulder vulder merged commit 864ff02 into vara-dev Nov 17, 2023
6 checks passed
@vulder vulder deleted the fix-configuration-generation branch November 17, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants