Skip to content

Add additional parenthesis for binary ops in pretty printer #27613

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 4 commits into from
Aug 21, 2015

Conversation

GSam
Copy link
Contributor

@GSam GSam commented Aug 9, 2015

In the case where there are no paren in the AST, the pretty printer doesn't correctly print binary operations where precedence is concerned. Parenthesis may be missing due to some kind of expansion or manipulation of the AST.

Example:
Pretty printer prints Expr(*, Expr(+, 1, 1), 2) as 1 + 1 * 2, as opposed to (1 + 1) * 2

r? @nrc

GSam added 3 commits August 9, 2015 21:15
Otherwise, we get (1 + 2) * 3 looking like 1 + 2 * 3
Previously it just added parentheses in excess.
e.g. ((1 + 2) + 3) + 4
This just adds paren following the same cases as before
@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@GSam
Copy link
Contributor Author

GSam commented Aug 9, 2015

Was failing on pretty/issue-4264.rs, due to some additional parenthesis. I assume that 'as' is left associative?

@nrc
Copy link
Member

nrc commented Aug 9, 2015

Rather than adding this logic in three places, could you modify print_expr_maybe_paren - these seem to be the only uses of that function and the name makes it sound like it should do what you want.

@GSam
Copy link
Contributor Author

GSam commented Aug 10, 2015

It doesn't seem much cleaner, especially with that Option, although perhaps I can do it a different way.

@nrc
Copy link
Member

nrc commented Aug 19, 2015

Can you check the sub-expression rather than passing in the parent?

@GSam
Copy link
Contributor Author

GSam commented Aug 20, 2015

@nrc Dropped the last patch.

@nrc
Copy link
Member

nrc commented Aug 21, 2015

@bors: r+

@bors
Copy link
Collaborator

bors commented Aug 21, 2015

📌 Commit 22baa46 has been approved by nrc

bors added a commit that referenced this pull request Aug 21, 2015
In the case where there are no paren in the AST, the pretty printer doesn't correctly print binary operations where precedence is concerned. Parenthesis may be missing due to some kind of expansion or manipulation of the AST. 

Example:
Pretty printer prints Expr(*, Expr(+, 1, 1), 2) as 1 + 1 * 2, as opposed to (1 + 1) * 2

r? @nrc
@bors
Copy link
Collaborator

bors commented Aug 21, 2015

⌛ Testing commit 22baa46 with merge 8f1b0aa...

@bors bors merged commit 22baa46 into rust-lang:master Aug 21, 2015
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