Skip to content

format(expr) beautification for quantifiers and = on booleans #2978

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
Sep 20, 2018

Conversation

kroening
Copy link
Member

No description provided.

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

This PR failed Diffblue compatibility checks (cbmc commit: fa434a5).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/85166064
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

  • the cbmc commit has disappeared in the mean time (e.g. in a force-push)
  • the author is not in the list of contributors (e.g. first-time contributors).

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

Passed Diffblue compatibility checks (cbmc commit: 3629a4a).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/85169864

else if(src.id() == ID_equal)
{
if(!src.operands().empty() && src.op0().type().id() == ID_bool)
operator_str = u8"\u21d4"; // <=>, U+21D4
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would confuse someone using the output to debug, since they'd go looking for ID_iff

Copy link
Member Author

Choose a reason for hiding this comment

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

Remove ID_iff?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's used only four times across the code base, that makes for an almost trivial PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kroening kroening force-pushed the format_expr_beautification branch from 3629a4a to be0aa32 Compare September 19, 2018 21:45
@kroening kroening requested a review from martin-cs as a code owner September 19, 2018 21:45
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

Passed Diffblue compatibility checks (cbmc commit: be0aa32).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/85359340

Daniel Kroening added 2 commits September 20, 2018 08:01
This makes the output closer to usual mathematical convention.
This makes the output closer to usual mathematical convention.
Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

The fact that user-visible output changes but no changes to tests were necessary implies that we don't actually test this code. This might change if #2977 were merged, so let's get that merged first.

@tautschnig tautschnig assigned kroening and unassigned tautschnig Sep 20, 2018
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

Passed Diffblue compatibility checks (cbmc commit: 37f5b8e).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/85388192

@kroening
Copy link
Member Author

@tautschnig This is now somewhat circular: to get #2977 merged, a lot of tests need to pass, and it's easier to achieve that if the output of format() is moved to be more similar to that what's there right now.
This PR was motivated by getting #2977 to merge.

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

Ok, let's add the tests in #2977 then.

@tautschnig tautschnig merged commit 83415a7 into develop Sep 20, 2018
@tautschnig tautschnig deleted the format_expr_beautification branch September 20, 2018 11:08
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