Skip to content

Libsyntax #1553

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 3 commits into from
May 17, 2017
Merged

Libsyntax #1553

merged 3 commits into from
May 17, 2017

Conversation

topecongiro
Copy link
Contributor

Closes #1542, closes #1541, closes #1538 and closes #1537.

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Thank you! Could you add some tests for catch blocks please?

src/expr.rs Outdated
// FIXME(#1537)
ast::ExprKind::Catch(..) => unimplemented!(),
// 6 = `catch `
ast::ExprKind::Catch(ref block) => block.rewrite(&context, try_opt!(shape.sub_width(6))),
Copy link
Member

Choose a reason for hiding this comment

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

I think the keywords are do catch, rather than just catch and you should explicitly emit these rather than rely on the missed span.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor Author

@topecongiro topecongiro May 15, 2017

Choose a reason for hiding this comment

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

I have few questions:

  1. Should do catch { ... } stay in a single line as unsafe when it is short?
  2. I found an example without do (let x = match catch { ... }). How should I treat these differently?
    I just found the above example is depricated. Sorry!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think do catch should follow unsafe. I don't think we should ever line-break between do and catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!
It would be nice if rustfmt could handle single line case inside impl Rewrite for ast::Block along with unsafe (here). However, as far as I know, ast::Block does not carry information whether it is catch block or not. Hence we need to handle single line case for catch before calling block.rewritehere. Is this correct?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that is correct (and unfortunate). I'm not sure how to best address it, to be honest. Perhaps pull the code out of the Rewrite impl and into a free function with an extra flag?

@nrc
Copy link
Member

nrc commented May 14, 2017

How does this fix the comments issue? (It looks like it does fix it from the test, but I don't understand how).

@topecongiro
Copy link
Contributor Author

The comments are inside macro_rules!, so the issue is fixed via using snippet from the span, rather than using the fallback path.

@topecongiro
Copy link
Contributor Author

topecongiro commented May 17, 2017

EDIT: rebased with the lastet libsyntax branch.

The latest commit implements catch, though it causes regression (please see tests/target/closure.rs).
Unfortunately, this is caused by fixing a small bug. Currently, when writing unsafe in a single line fail, rustfmt does not try multiline because it returns earlier via try_opt! (here). The commit fixes this bug by try_opt!ing in a different function.

@nrc nrc merged commit f437cac into rust-lang:libsyntax May 17, 2017
@nrc
Copy link
Member

nrc commented May 17, 2017

Looks good, thank you!

@topecongiro topecongiro deleted the libsyntax branch August 1, 2017 14:58
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.

2 participants