-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Suggest removing leading left angle brackets. #57852
Conversation
I'm not sure how this interacts with #57817 if there are both extra trailing and leading angle brackets. |
A cursory look at the code tells me that's fine, but I wonder how often this case happens as opposed to missing trailing brackets. What's the output of this PR for the following?
|
This commit adds errors and accompanying suggestions as below: ``` bar::<<<<<T as Foo>::Output>(); ^^^ help: remove extra angle brackets ```
7dfd03c
to
22f794b
Compare
I've put the output of this PR as well as the output of this PR and #57817 at the same time in this gist. I think this behaves appropriately. |
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 am slightly worried about the snapshot usage here (which again, it might be fine), but I'm thinking that we might be able to use unmatched_angle_bracket_count
in expected_one_of
...
@bors try |
⌛ Trying commit 22f794b with merge 100d423c5c506b60b6ab591dfea5515f65a7fb74... |
By the way, I love the level of recovery. With the introduction of
|
This commit implements a suggestion from @estebank that optimizes the use of snapshots. Instead of creating a snapshot for each recursion in `parse_path_segment` and then replacing `self` with them until the first invocation where if leading angle brackets are detected, `self` is not replaced and instead the snapshot is used to inform how parsing should continue. Now, a snapshot is created in the first invocation that acts as a backup of the parser state before any generic arguments are parsed (and therefore, before recursion starts). This backup replaces `self` if after all parsing of generic arguments has concluded we can determine that there are leading angle brackets. Parsing can then proceed from the backup state making use of the now known number of unmatched leading angle brackets to recover.
☀️ Test successful - checks-travis |
@bors try |
⌛ Trying commit 8ab12f6 with merge de98dc84921c98aec5fe83cceb4cda42ba22e555... |
@rust-timer build 100d423c5c506b60b6ab591dfea5515f65a7fb74 |
☀️ Test successful - checks-travis |
@rust-timer build de98dc84921c98aec5fe83cceb4cda42ba22e555 |
Insufficient permissions to issue commands to rust-timer. |
@rust-timer build de98dc84921c98aec5fe83cceb4cda42ba22e555 I've also added you to the list |
Success: Queued de98dc84921c98aec5fe83cceb4cda42ba22e555 with parent 19f8958, comparison URL. |
1 similar comment
Success: Queued de98dc84921c98aec5fe83cceb4cda42ba22e555 with parent 19f8958, comparison URL. |
@rust-lang/compiler I'm looking at the perf results and it looks reasonable to me for the most part, but there are some 5% worst case swings in |
My suspicion, @estebank, is that these results are mostly noise. This is my reasoning, let me know if you see a flaw:
For example, consider these results:
Here, I would think that if the parser got slower, and it was affecting the "baseline incremental" run (which means the first incremental run, with no prior state), then it ought to affect "clean incremental" even more so -- as clean incremental does the same amount of parsing, but does a lot less other work. So the main question is whether I am correct that the overhead here would be focused on the parser, I guess. If so, I think it's fine. |
I agree that this is only parser overhead. The only regression that seemed relevant was memory usage for webrender-debug for a clean compile of 9% , but even that seems a bit high. Regardless, the parser is a small portion of where @bors r+ |
📌 Commit 8ab12f6 has been approved by |
Suggest removing leading left angle brackets. Fixes rust-lang#57819. This PR adds errors and accompanying suggestions as below: ``` bar::<<<<<T as Foo>::Output>(); ^^^ help: remove extra angle brackets ``` r? @estebank
Suggest removing leading left angle brackets. Fixes rust-lang#57819. This PR adds errors and accompanying suggestions as below: ``` bar::<<<<<T as Foo>::Output>(); ^^^ help: remove extra angle brackets ``` r? @estebank
☀️ Test successful - checks-travis, status-appveyor |
Fixes #57819.
This PR adds errors and accompanying suggestions as below:
r? @estebank