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

rustc_ast: Harmonize delimiter naming with proc_macro::Delimiter #96433

Merged
merged 1 commit into from
Apr 28, 2022
Merged

rustc_ast: Harmonize delimiter naming with proc_macro::Delimiter #96433

merged 1 commit into from
Apr 28, 2022

Conversation

petrochenkov
Copy link
Contributor

Compiler cannot reuse proc_macro::Delimiter directly due to extra impls, but can at least use the same naming.

After this PR the only difference between these two enums is that proc_macro::Delimiter::None is turned into token::Delimiter::Invisible.
It's my mistake that the invisible delimiter is called None on stable, during the stabilization I audited the naming and wrote the docs, but missed the fact that the None naming gives a wrong and confusing impression about what this thing is.

cc #96421
r? @nnethercote

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 26, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/rustfmt.

cc @rust-lang/rustfmt

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
nnethercote
nnethercote reviewed Apr 26, 2022
View reviewed changes
compiler/rustc_ast_pretty/src/pprust/state.rs
token::OpenDelim(Delimiter::Brace) => "{".into(),
token::CloseDelim(Delimiter::Brace) => "}".into(),
token::OpenDelim(Delimiter::Invisible) | token::CloseDelim(Delimiter::Invisible) => {
"".into()
Copy link
Contributor

Choose a reason for hiding this comment

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

Off-topic: I was thinking about rendering these as something like /*⟪*/ and /*⟫*/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, but I think it would require #96421 to land, to avoid printing this in #[word] and #[key = "value"] attributes.

nnethercote
nnethercote reviewed Apr 26, 2022
View reviewed changes
Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

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

r=me with the comment below addressed.

@bors delegate+

@nnethercote
Copy link
Contributor

I'm now also wondering if "implicit" is better than "invisible", mostly because I just read https://github.com/rust-lang/reference/blob/master/src/procedural-macros.md which uses "implicit". But then, I see that https://docs.rs/syn/latest/syn/ uses "invisible". I'll let you decide :)

@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 Apr 27, 2022
@petrochenkov
Copy link
Contributor Author

I think the syn's naming is more suitable, so I left it as Invisible.
@bors r=nnethercote

@bors
Copy link
Contributor

bors commented Apr 27, 2022

📌 Commit ce00327784d2dcfbc1db6478a555ee8c69c4c8b4 has been approved by nnethercote

@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 Apr 27, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 28, 2022
Closed
@bors

This comment was marked as resolved.

Sign in to view
@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 28, 2022
@petrochenkov
Copy link
Contributor Author

@bors r=nnethercote

@bors
Copy link
Contributor

bors commented Apr 28, 2022

📌 Commit 2733ec1 has been approved by nnethercote

@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 Apr 28, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 28, 2022
@Dylan-DPC
Rollup merge of rust-lang#96433 - petrochenkov:delim, r=nnethercote
d6905ce 
rustc_ast: Harmonize delimiter naming with `proc_macro::Delimiter`

Compiler cannot reuse `proc_macro::Delimiter` directly due to extra impls, but can at least use the same naming.

After this PR the only difference between these two enums is that `proc_macro::Delimiter::None` is turned into `token::Delimiter::Invisible`.
It's my mistake that the invisible delimiter is called `None` on stable, during the stabilization I audited the naming and wrote the docs, but missed the fact that the `None` naming gives a wrong and confusing impression about what this thing is.

cc rust-lang#96421
r? `@nnethercote`
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 28, 2022
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 28, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2022
@bors
Auto merge of rust-lang#96528 - Dylan-DPC:rollup-iedbjli, r=Dylan-DPC
e85edd9 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#95312 (Ensure that `'_` and GAT yields errors)
 - rust-lang#96405 (Migrate ambiguous plus diagnostic to the new derive macro)
 - rust-lang#96409 (Recover suggestions to introduce named lifetime under NLL)
 - rust-lang#96433 (rustc_ast: Harmonize delimiter naming with `proc_macro::Delimiter`)
 - rust-lang#96480 (Fixed grammatical error in example comment)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0cbf3b2 into rust-lang:master Apr 28, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 28, 2022
@dtolnay dtolnay mentioned this pull request Apr 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nnethercote nnethercote nnethercote left review comments

Assignees

@nnethercote nnethercote

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@petrochenkov @rust-highfive @rust-log-analyzer @nnethercote @bors @rustbot