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.

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

refactor lexer to use idiomatic borrowing #62124

Merged
merged 2 commits into from Jun 28, 2019
Merged

Conversation

matklad
Copy link
Member

@matklad matklad commented Jun 25, 2019

No description provided.

@matklad
Copy link
Member Author

matklad commented Jun 25, 2019

r? @petrochenkov

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2019
@matklad
Copy link
Member Author

matklad commented Jun 25, 2019

One thing I find confusing is

    fn name_from(&self, start: BytePos) -> ast::Name {
        debug!("taking an ident from {:?} to {:?}", start, self.pos);
        Symbol::intern(self.str_from(start))
    }

It's not obvious that ast::Name and SYmbol are aliases. Perhaps something like

    fn intern_from(&self, start: BytePos) -> Symbol {
        debug!("taking an ident from {:?} to {:?}", start, self.pos);
        Symbol::intern(self.str_from(start))
    }

would be better? If so, it makes sense to do this in this PR as well

@petrochenkov
Copy link
Contributor

petrochenkov commented Jun 25, 2019

@matklad
ast::Name is conventionally used for identifier symbols from ast::Ident (although people just use Symbol randomly now).

In this case the symbols are not necessarily identifiers, so they shouldn't use Name/name.
(I'd rather use symbol_from than intern_from).

@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 Jun 25, 2019
@matklad
Copy link
Member Author

matklad commented Jun 25, 2019

Makes sense, added the second commit with rename

@matklad
Copy link
Member Author

matklad commented Jun 25, 2019

... and replaced the few remaning usages of ast::Name in the lexer: it's hard to maintain convention unless it is of the form "always do X" :)

Lexer uses Symbols for a lot of stuff, not only for identifiers, so
the "name" terminology is just confusing.
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 25, 2019

📌 Commit 57db25e 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 Jun 25, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 25, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 26, 2019
bors added a commit that referenced this pull request Jun 26, 2019
Rollup of 8 pull requests

Successful merges:

 - #60340 (Rename .cap() methods to .capacity())
 - #61767 (Update new_debug_unreachable)
 - #62043 (Remove `FnBox`)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62104 (Inform the query system about properties of queries at compile time)
 - #62106 (Add more tests for async/await)
 - #62124 (refactor lexer to use idiomatic borrowing)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Jun 26, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 27, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 27, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 27, 2019
bors added a commit that referenced this pull request Jun 28, 2019
Rollup of 16 pull requests

Successful merges:

 - #61878 (improve pinning projection docs)
 - #62043 (Remove `FnBox`)
 - #62067 (Add suggestion for missing `.await` keyword)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62106 (Add more tests for async/await)
 - #62124 (refactor lexer to use idiomatic borrowing)
 - #62131 (libsyntax: Fix some Clippy warnings)
 - #62152 (Don't ICE on item in `.await` expression)
 - #62154 (Remove old fixme)
 - #62155 (Add regression test for MIR drop generation in async loops)
 - #62156 (Update books)
 - #62160 (Remove outdated question_mark_macro_sep lint)
 - #62164 (save-analysis: use buffered writes)
 - #62171 (rustc: Retry SIGILL linker invocations)
 - #62176 (Update RLS)

Failed merges:

r? @ghost
@bors bors merged commit 57db25e into rust-lang:master Jun 28, 2019
@matklad matklad deleted the without-with branch June 28, 2019 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants