Skip to content

Conversation

@Veetaha
Copy link
Contributor

@Veetaha Veetaha commented Jan 22, 2020

Just a small refactoring along the way of reading the codebase

// and the comments on the linked PR.
let float_suffix_list = ["f32", "f64"];
pub fn kind(&self) -> LiteralKind {
const INT_SUFFIXES: [&'static str; 12] = [
Copy link
Contributor Author

@Veetaha Veetaha Jan 22, 2020

Choose a reason for hiding this comment

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

FYI: There is a caveat.
If the language introduces new suffixes or user-defined suffixes (who knows)
You should ensure that suffixes in these arrays do not include each other as a substring at the end or replace .ends_with()
test with something more elaborated (or just order the suffixes properly to avoid a bug)

@Veetaha Veetaha requested a review from matklad January 22, 2020 00:15
.map(|&suffix| SmolStr::new(suffix))
}

// The lexer treats e.g. `1f64` as an integer literal. See
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is useful. Without the comment it’s not necessarily obvious why we’d check float suffixes for a token of kind INT_NUMBER (and indeed the code incorrectly didn’t do this before the PR in which the comment was added).

Copy link
Member

Choose a reason for hiding this comment

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

Nit (in case you touch this code again): "below".

Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

bors r+

Nice cleanup!

Also, big 👍 on the general refactoring work, rust-analyzer can use a lot of that!

@Veetaha
Copy link
Contributor Author

Veetaha commented Jan 22, 2020

Hmm, strange, it runs well on my PC

@matklad
Copy link
Contributor

matklad commented Jan 22, 2020

bors retry

That's a known bug on windows (which existed before we've enabled CI on windows). This should pass CI.

#2835

@matklad
Copy link
Contributor

matklad commented Jan 22, 2020

@Veetaha could you rebase this? Due to another issue with github actions/bors, I think this won't get past the bors.

bors d+

@bors
Copy link
Contributor

bors bot commented Jan 22, 2020

✌️ Veetaha can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@Veetaha
Copy link
Contributor Author

Veetaha commented Jan 22, 2020

bors retry please

@bors
Copy link
Contributor

bors bot commented Jan 22, 2020

✌️ Veetaha can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@Veetaha
Copy link
Contributor Author

Veetaha commented Jan 22, 2020

Thank you bors, now merge

@Veetaha
Copy link
Contributor Author

Veetaha commented Jan 22, 2020

bors merge

bors bot added a commit that referenced this pull request Jan 22, 2020
2891: ra_syntax: removed code duplication and token reevaluation r=Veetaha a=Veetaha

Just a small refactoring along the way of reading the codebase

Co-authored-by: Veetaha <gerzoh1@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jan 22, 2020

Build succeeded

  • Rust (macos-latest)
  • Rust (ubuntu-latest)
  • TypeScript

@bors bors bot merged commit fa31841 into rust-lang:master Jan 22, 2020
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