Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Literal error reporting cleanup #72047

Merged
merged 4 commits into from
May 16, 2020
Merged

Literal error reporting cleanup #72047

merged 4 commits into from
May 16, 2020

Conversation

Julian-Wollersberger
Copy link
Contributor

While doing some performance work, I noticed some code duplication in librustc_parser/lexer/mod.rs, so I cleaned it up.

This PR is probably best reviewed commit by commit.

I'm not sure what the API stability practices for librustc_lexer are. Four public methods in unescape.rs can be removed, but two are used by clippy, so I left them in for now.
I could open a PR for Rust-Analyzer when this one lands.

But how do I open a PR for clippy? (Git submodules are frustrating to work with)

@Julian-Wollersberger
Small doc improvements.
e734e31 
The phrasing is from the commit description of 395ee0b by @matklad.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (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. Due to 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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 9, 2020
@petrochenkov
Copy link
Contributor

r? @petrochenkov
cc @matklad

@rust-highfive rust-highfive assigned petrochenkov and unassigned eddyb May 9, 2020
@petrochenkov
Copy link
Contributor

But how do I open a PR for clippy? (Git submodules are frustrating to work with)

clippy is currently using a subtree rather than a submodule, so it can be fixed right in this repo.

@Julian-Wollersberger
Copy link
Contributor Author

Thanks. It didn't work with CLion, but it did with command line git.

@rust-highfive

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor 

./x.py test --stage 0 src/librustc_lexer

to quickly run the unit tests that are currently failing on CI.

petrochenkov
petrochenkov reviewed May 10, 2020
View reviewed changes
src/librustc_parse/lexer/mod.rs Outdated Show resolved Hide resolved
src/librustc_parse/lexer/mod.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

LGTM.
(The unescape_* commits could use some squashing.)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2020
@Julian-Wollersberger
Copy link
Contributor Author

Addressed the feedback (and remembered to do x.py fmt and test tidy this time).
Thanks for reviewing.

@petrochenkov
Copy link
Contributor

See #72047 (comment), but I'll r+ in a couple of days anyway because that's not super critical.

@Julian-Wollersberger
Unified unescape_{char,byte,str,byte_str,raw_str,raw_byte_str} meth…
1be5d1e 
…ods into one method `unescape_literal` with a mode argument.
@Julian-Wollersberger
Unified validate_{byte,str,raw_str,raw_byte_str}_escape methods int…
18cc63d 
…o one method `validate_literal_escape` with a mode argument.

This enables simplifying the `match` in `cook_lexer_literal()`
and it eliminates 90 lines of repetition :)
@Julian-Wollersberger
Replace some usages of the old unescape_ functions in AST, clippy a…
43ae785 
…nd tests.
@Julian-Wollersberger
Copy link
Contributor Author

Cleaned up the git history and the code for Int and Float isn't moved around anymore.

@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented May 13, 2020

📌 Commit 43ae785 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 13, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request May 15, 2020
@RalfJung
Rollup merge of rust-lang#72047 - Julian-Wollersberger:literal_error_…
1df77aa 
…reporting_cleanup, r=petrochenkov

Literal error reporting cleanup

While doing some performance work, I noticed some code duplication in `librustc_parser/lexer/mod.rs`, so I cleaned it up.

This PR is probably best reviewed commit by commit.

I'm not sure what the API stability practices for `librustc_lexer` are. Four public methods in `unescape.rs` can be removed, but two are used by clippy, so I left them in for now.
I could open a PR for Rust-Analyzer when this one lands.

But how do I open a PR for clippy? (Git submodules are frustrating to work with)
@RalfJung RalfJung mentioned this pull request May 15, 2020
Closed
@RalfJung RalfJung mentioned this pull request May 16, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 16, 2020
@bors
Auto merge of rust-lang#72276 - RalfJung:rollup-nkfu3is, r=RalfJung
dd927a5 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#72045 (Incomplete features can also be unsound)
 - rust-lang#72047 (Literal error reporting cleanup)
 - rust-lang#72060 (move `ty::List` into a new submodule)
 - rust-lang#72094 (cmdline: Make target features individually overridable)
 - rust-lang#72254 (Remove redundant backtick in error message.)

Failed merges:

r? @ghost
@bors bors merged commit ec5610f into rust-lang:master May 16, 2020
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 17, 2020
@RalfJung
Rollup merge of rust-lang#72047 - Julian-Wollersberger:literal_error_…
8bba1b7 
…reporting_cleanup, r=petrochenkov

Literal error reporting cleanup

While doing some performance work, I noticed some code duplication in `librustc_parser/lexer/mod.rs`, so I cleaned it up.

This PR is probably best reviewed commit by commit.

I'm not sure what the API stability practices for `librustc_lexer` are. Four public methods in `unescape.rs` can be removed, but two are used by clippy, so I left them in for now.
I could open a PR for Rust-Analyzer when this one lands.

But how do I open a PR for clippy? (Git submodules are frustrating to work with)
@Julian-Wollersberger Julian-Wollersberger mentioned this pull request May 24, 2020
Merged
bors bot added a commit to rust-lang/rust-analyzer that referenced this pull request May 24, 2020
@bors @Julian-Wollersberger
Merge #4590
cc6ccf7 
4590: Update to rustc_lexer version 660 r=matklad a=Julian-Wollersberger

Change the `unescape_*()` functions to `unescape_literal()`, to address the canges I made in rust-lang/rust#72047.

I also noticed some outdated FIXMEs.



Co-authored-by: Julian Wollersberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Julian-Wollersberger @rust-highfive @petrochenkov @bors @eddyb