2

Reimplement C-str literals by fee1-dead · Pull Request #113476 · rust-lang/rust...

 1 year ago
source link: https://github.com/rust-lang/rust/pull/113476
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Reimplement C-str literals #113476

Conversation

Member

This reverts #113334, cc @fmease.

While converting lexer tokens to ast Tokens in rustc_parse, we check the edition of the span of the token. If the edition < 2021, we split the token into two, one being the identifier and other being the str literal.

Fixes #113333

Veykril reacted with confused emojifmease reacted with heart emoji

Collaborator

r? @WaffleLapkin

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

rustbot

added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

labels

Jul 8, 2023

Collaborator

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

This comment has been minimized.

Collaborator

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Contributor

we pass is_rust_2021 as a boolean flag to the lexer

This sounds like a red flag, and if it is required, then the previous literal prefix future-proofing was probably done incorrectly.
I'll check in a couple of days.

Contributor

klensy

commented

Jul 8, 2023

•

edited

Basically we pass is_rust_2021 as a boolean flag

So in edition > 2021 this will be false and broken? For example edition 2024.
Ugh, sess.edition.rust_2021() actually means edition >= 2021, thats unexpected.

Member

Author

That flag will be true for 2024. Should I rename?

Contributor

we pass is_rust_2021 as a boolean flag to the lexer

This sounds like a red flag, and if it is required, then the previous literal prefix future-proofing was probably done incorrectly. I'll check in a couple of days.

The future-proofing is correct.
fn next_token in the parser pulls the next token from the lexer, applies a span to it (including hygiene information which contains edition) and report_unknown_prefix processes it and reports or not reports the error.

If this PR worked consistently with the existing future-proofing check, then it would either:

  • In fn next_token pull the whole C-str literal and then break it into two tokens if the edition is 2015-2018.
  • In fn next_token pull the prefix and then additionally pull the rest of the C-str literal and combine, if the prefix's edition is 2021+.

Both variants may require changes to the lexer, but neither of them requires passing an edition to it.
We can implement any of these alternatives that is simpler to implement in practice.

Contributor

That flag will be true for 2024. Should I rename?

If this is implemented in lexer (rather than as post-processing in fn next_token), then I'd rather rename it to something like enable_c_str to make the lexer configurable, but still edition-oblivious.

Member

Author

I'll try implementing edition-based token combining then.

Member

Author

implemented edition-based token splitting instead

petrochenkov

added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jul 16, 2023

petrochenkov

removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label

Jul 17, 2023

petrochenkov

added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label

Jul 17, 2023

This comment has been minimized.

petrochenkov

added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jul 23, 2023

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

Jul 24, 2023

Contributor

Thanks!
@bors r+
fee1-dead reacted with heart emoji

Contributor

pushpin Commit a0376e9 has been approved by petrochenkov

It is now in the queue for this repository.

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

Jul 25, 2023

Contributor

hourglass Testing commit a0376e9 with merge 23405bb...

Contributor

Could you add "Fixes #113333" to the PR description since this PR doesn't need backporting?

Member

Author

Could you add "Fixes #113333" to the PR description since this PR doesn't need backporting?
fmease reacted with thumbs up emoji

Contributor

sunny Test successful - checks-actions
Approved by: petrochenkov
Pushing 23405bb to master...

bors

added the merged-by-bors This PR was explicitly merged by bors label

Jul 25, 2023

bors

merged commit 23405bb into

rust-lang:master

Jul 25, 2023

12 checks passed

Collaborator

Finished benchmarking commit (23405bb): comparison URL.

Overall result: x regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.7% [0.7%, 0.7%] 3
Regressions ❌
(secondary)		 0.7% [0.5%, 1.2%] 3
Improvements white_check_mark
(primary)		 - - 0
Improvements white_check_mark
(secondary)		 - - 0
All xwhite_check_mark (primary) 0.7% [0.7%, 0.7%] 3

Max RSS (memory usage)

Results

Cycles

Results

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 651.831s -> 651.438s (-0.06%)

Member

Perf: this is noise

@rustbot label: +perf-regression-triaged

rustbot

added the perf-regression-triaged The performance regression has been triaged. label

Aug 5, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

petrochenkov

petrochenkov left review comments

fmease

fmease left review comments

Nilstrieb

Nilstrieb left review comments
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors perf-regression Performance regressions perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects

None yet

Milestone

1.73.0

Development

Successfully merging this pull request may close these issues.

Reimplement the lexing of c"…" string literals with backward compatibility in mind

11 participants

report

Recommend

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK