Skip to content

Unit tests for simplifying boolean and cast expressions #5229

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

Merged
merged 2 commits into from
Feb 11, 2020

Conversation

thk123
Copy link
Contributor

@thk123 thk123 commented Feb 7, 2020

  • Each commit message has a non-empty body, explaining why the change was made.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • [na] The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • [na] My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

Added some new unit tests to check simplification of boolean formula (so function can be used as basis for testing #5200) Also imported and updated the tests I wrote in #1075.

@tautschnig I notice in #1075 you seemed to say the following shouldn't be simplified:

It was because I'd left long type as the same as int type, so was redundant. Now that's corrected, the tests behave as expected:

  • can remove casts of constant values
  • do not remove casts on symbols, even when target it larger

@thk123 thk123 requested review from tautschnig and xbauch February 7, 2020 15:27
@thk123 thk123 changed the title Test simplify boolean expr Unit tests for simplifying boolean and cast expressions Feb 7, 2020
}
SECTION("Nested boolean expressions")
{
INFO("((!true) || true) && (false => false))")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
INFO("((!true) || true) && (false => false))")
INFO("((!true) || (false => false)) && true)")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙇‍♂️

{
struct test_entryt
{
irep_idt comparision;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
irep_idt comparision;
irep_idt comparison;

const auto binary_relation_from = [](const test_entryt &entry) {
const signedbv_typet int_type(32);
return binary_relation_exprt{from_integer(entry.lhs, int_type),
entry.comparision,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
entry.comparision,
entry.comparison,

symbol_tablet symbol_table;
namespacet ns(symbol_table);
const auto int_type = signedbv_typet(32);
const auto long_type = signedbv_typet(32);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that int and long are of the same width?

Copy link
Contributor

@xbauch xbauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except that I'm not sure if casting to a smaller type should be simplified.

@thk123 thk123 force-pushed the test-simplify-boolean-expr branch from 281a155 to a8c1571 Compare February 7, 2020 16:26
@thk123
Copy link
Contributor Author

thk123 commented Feb 7, 2020

@xbauch thank you for your careful eye, comments addressed

Copy link
Contributor

@xbauch xbauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Copy link
Collaborator

@martin-cs martin-cs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tidying up old commits.

@thk123
Copy link
Contributor Author

thk123 commented Feb 11, 2020

Restarted one CI job - looks spurious (apt get failure)

@thk123 thk123 merged commit 50fa5a2 into diffblue:develop Feb 11, 2020
@thk123 thk123 deleted the test-simplify-boolean-expr branch February 11, 2020 10:30
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.

3 participants