Skip to content

Use tidy to sort sym::* items #143110

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yotamofek
Copy link
Contributor

@yotamofek yotamofek commented Jun 27, 2025

Use tidy to sort the symbols in the invocation of symbols!, instead of implementing the ordering check inside the proc macro.

(asked @nnethercote about this on zulip, he didn't have any reservations about making this change)

This has a couple of benefits:

  • tidy's "version sort" (thanks to make tidy-alphabetical use a natural sort #141311 !) is nicer than the naive-cmp sort, so, e.g. AtomicI{8, 16, 32, 64, 128} are properly sorted by bit width.
  • consistency with the rest of the repo
  • allows us to remove a bit of order-verifying code from the symbols! proc macro impl

@rustbot
Copy link
Collaborator

rustbot commented Jun 27, 2025

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 27, 2025
@rust-log-analyzer

This comment has been minimized.

@yotamofek yotamofek force-pushed the pr/tidy-sort-for-symbols branch from cb27d40 to 7a3a7c0 Compare June 27, 2025 18:50
@scottmcm
Copy link
Member

Looks good to me! Treating it as a formatting problem instead of a macro error sounds generally good anyway, and I agree the version-sort order is nicer in the places that changed.

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 27, 2025

📌 Commit 7a3a7c0 has been approved by scottmcm

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2025
@scottmcm scottmcm assigned scottmcm and unassigned BoxyUwU Jun 27, 2025
@nnethercote
Copy link
Contributor

This is good, but I have a couple of nits I'd like addressed, so for now:

@bors r-

@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 Jun 28, 2025
@@ -158,6 +158,7 @@ symbols! {
// There is currently no checking that all symbols are used; that would be
Copy link
Contributor

Choose a reason for hiding this comment

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

The above paragraph is now out-of-date:

  • The proc macro will only abort if symbols are duplicated (I assume the duplication detection still works?)
  • The Vim instructions are no longer correct and should just be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Once that's addressed it will be good to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, missed that! Removed the whole paragraph.

I assume the duplication detection still works?

Yep.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did something go wrong? It looks like the paragraph is still there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh woops 🫢
Sorry about that.
I think I haven't completely woken up yet today 😅

@yotamofek yotamofek force-pushed the pr/tidy-sort-for-symbols branch from 7a3a7c0 to bd91ca0 Compare June 28, 2025 08:19
@yotamofek yotamofek force-pushed the pr/tidy-sort-for-symbols branch from bd91ca0 to 00b64f8 Compare June 28, 2025 09:26
@nnethercote
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jun 28, 2025

📌 Commit 00b64f8 has been approved by nnethercote

It is now in the queue for this repository.

@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 28, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2025
…, r=nnethercote

Use tidy to sort `sym::*` items

Use tidy to sort the symbols in the invocation of `symbols!`, instead of implementing the ordering check inside the proc macro.

(asked `@nnethercote` about this on zulip, he didn't have any reservations about making this change)

This has a couple of benefits:
- tidy's "version sort" (thanks to rust-lang#141311 !) is nicer than the naive-cmp sort, so, e.g. `AtomicI{8, 16, 32, 64, 128}` are properly sorted by bit width.
- consistency with the rest of the repo
- allows us to remove a bit of order-verifying code from the `symbols!` proc macro impl
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2025
…, r=nnethercote

Use tidy to sort `sym::*` items

Use tidy to sort the symbols in the invocation of `symbols!`, instead of implementing the ordering check inside the proc macro.

(asked ``@nnethercote`` about this on zulip, he didn't have any reservations about making this change)

This has a couple of benefits:
- tidy's "version sort" (thanks to rust-lang#141311 !) is nicer than the naive-cmp sort, so, e.g. `AtomicI{8, 16, 32, 64, 128}` are properly sorted by bit width.
- consistency with the rest of the repo
- allows us to remove a bit of order-verifying code from the `symbols!` proc macro impl
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2025
…, r=nnethercote

Use tidy to sort `sym::*` items

Use tidy to sort the symbols in the invocation of `symbols!`, instead of implementing the ordering check inside the proc macro.

(asked ```@nnethercote``` about this on zulip, he didn't have any reservations about making this change)

This has a couple of benefits:
- tidy's "version sort" (thanks to rust-lang#141311 !) is nicer than the naive-cmp sort, so, e.g. `AtomicI{8, 16, 32, 64, 128}` are properly sorted by bit width.
- consistency with the rest of the repo
- allows us to remove a bit of order-verifying code from the `symbols!` proc macro impl
bors added a commit that referenced this pull request Jun 28, 2025
Rollup of 10 pull requests

Successful merges:

 - #123476 (std::net: adding `unix_socket_exclbind` feature for solaris/illumos.)
 - #142708 (Do not include NUL-terminator in computed length)
 - #142963 (Skip unnecessary components in x64 try builds)
 - #142987 (rustdoc: show attributes on enum variants)
 - #143031 (Add windows-gnullvm hosts to the manifest)
 - #143082 (update internal `send_signal` comment)
 - #143110 (Use tidy to sort `sym::*` items)
 - #143111 (BTreeSet: remove duplicated code by reusing `from_sorted_iter`)
 - #143114 (Minor Documentation Improvements)
 - #143137 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2025
…, r=nnethercote

Use tidy to sort `sym::*` items

Use tidy to sort the symbols in the invocation of `symbols!`, instead of implementing the ordering check inside the proc macro.

(asked ````@nnethercote```` about this on zulip, he didn't have any reservations about making this change)

This has a couple of benefits:
- tidy's "version sort" (thanks to rust-lang#141311 !) is nicer than the naive-cmp sort, so, e.g. `AtomicI{8, 16, 32, 64, 128}` are properly sorted by bit width.
- consistency with the rest of the repo
- allows us to remove a bit of order-verifying code from the `symbols!` proc macro impl
bors added a commit that referenced this pull request Jun 28, 2025
Rollup of 10 pull requests

Successful merges:

 - #123476 (std::net: adding `unix_socket_exclbind` feature for solaris/illumos.)
 - #142708 (Do not include NUL-terminator in computed length)
 - #142963 (Skip unnecessary components in x64 try builds)
 - #142974 (Update stage0 to 1.89.0-beta.1)
 - #142987 (rustdoc: show attributes on enum variants)
 - #143031 (Add windows-gnullvm hosts to the manifest)
 - #143082 (update internal `send_signal` comment)
 - #143110 (Use tidy to sort `sym::*` items)
 - #143111 (BTreeSet: remove duplicated code by reusing `from_sorted_iter`)
 - #143114 (Minor Documentation Improvements)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2025
…, r=nnethercote

Use tidy to sort `sym::*` items

Use tidy to sort the symbols in the invocation of `symbols!`, instead of implementing the ordering check inside the proc macro.

(asked `````@nnethercote````` about this on zulip, he didn't have any reservations about making this change)

This has a couple of benefits:
- tidy's "version sort" (thanks to rust-lang#141311 !) is nicer than the naive-cmp sort, so, e.g. `AtomicI{8, 16, 32, 64, 128}` are properly sorted by bit width.
- consistency with the rest of the repo
- allows us to remove a bit of order-verifying code from the `symbols!` proc macro impl
bors added a commit that referenced this pull request Jun 28, 2025
Rollup of 9 pull requests

Successful merges:

 - #123476 (std::net: adding `unix_socket_exclbind` feature for solaris/illumos.)
 - #142708 (Do not include NUL-terminator in computed length)
 - #142963 (Skip unnecessary components in x64 try builds)
 - #142987 (rustdoc: show attributes on enum variants)
 - #143031 (Add windows-gnullvm hosts to the manifest)
 - #143082 (update internal `send_signal` comment)
 - #143110 (Use tidy to sort `sym::*` items)
 - #143111 (BTreeSet: remove duplicated code by reusing `from_sorted_iter`)
 - #143114 (Minor Documentation Improvements)

r? `@ghost`
`@rustbot` modify labels: rollup

try-job: dist-i586-gnu-i586-i686-musl
bors added a commit that referenced this pull request Jun 28, 2025
Rollup of 9 pull requests

Successful merges:

 - #123476 (std::net: adding `unix_socket_exclbind` feature for solaris/illumos.)
 - #142708 (Do not include NUL-terminator in computed length)
 - #142963 (Skip unnecessary components in x64 try builds)
 - #142987 (rustdoc: show attributes on enum variants)
 - #143031 (Add windows-gnullvm hosts to the manifest)
 - #143082 (update internal `send_signal` comment)
 - #143110 (Use tidy to sort `sym::*` items)
 - #143111 (BTreeSet: remove duplicated code by reusing `from_sorted_iter`)
 - #143114 (Minor Documentation Improvements)

r? `@ghost`
`@rustbot` modify labels: rollup

try-job: dist-i586-gnu-i586-i686-musl
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants