-
Notifications
You must be signed in to change notification settings - Fork 18
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 some access levels in MainSolver
and add some checks to public …
#689
Conversation
Making |
dc42738
to
d001afa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in MainSolver are good, but I suggest to deal with UFInterpolationTest
differently and make it consistent with how LRAInterpolationTest
works.
test/unit/test_UFInterpolation.cc
Outdated
@@ -11,6 +11,12 @@ bool verifyInterpolant(PTRef itp, PartitionManager & pManager, ipartitions_t con | |||
return VerificationUtils(logic).verifyInterpolantInternal(pManager.getPartition(Amask, PartitionManager::part::A), pManager.getPartition(Amask, PartitionManager::part::B), itp); | |||
} | |||
|
|||
struct TestMainSolver : MainSolver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly like the approach of creating subclasses solely for the purpose being able to access some protected member.
I think a better solution here would be to do what LRAInterpolationTest
is doing, i.e., explicitly track the partitions in each test.
That way the public API is sufficient.
1d2cdc3
to
eb7ef02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Please, next time, make sure that the title of git commit is limited to 72 characters, so that GitHub does not split it when creating a PR. |
…member functions.
Related to #688 (does not fully resolve)