Skip to content

Eliminate word_and_empty methods. #140095

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
May 9, 2025
Merged

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Apr 21, 2025

To remove the last remaining Ident::empty uses.

r? @jdonszelmann

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) 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 Apr 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 21, 2025

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

@nnethercote
Copy link
Contributor Author

nnethercote commented Apr 21, 2025

An opinionated PR, this one... sorry there are a bunch of changes in a single commit, it was hard to separate them out. Removing the functions that returned tuples was important, they got in the way of other changes.

@jdonszelmann
Copy link
Contributor

I see, well I'll take a look :)

@nnethercote
Copy link
Contributor Author

This helps with #137978.

@jdonszelmann
Copy link
Contributor

hm, would you mind at least making the removals from parser.rs a separate commit. I'm not 100% sure I agree with those removals. I intended those APIs to be for all the future attribute parsers that will be converted to a dedicated parser implementation. The reason I am kind of okay with it is because that some of those APIs were made before I allowed easy matching on attribute parsers, which really is a better API so maybe that's what we should switch to. But that'd still be nicer to discuss/refer to if it were a separate commit. I also quickly want to see whether some of my open but blocked PRs relied on those APIs just to see if I had very good reasons for them but I doubt it.

@nnethercote
Copy link
Contributor Author

I have split it into two commits.

@jdonszelmann
Copy link
Contributor

The first commit is blocking another PR. That's the one I reviewed and am happy with merging (maybe after some nitpicks are addressed). I think some of those unwraps might be unnecessary. I can take one more look tomorrow if you decide to change something and then we can merge it. The other, second, commit I have some doubts about right now, which I expressed earlier. If it helps unblock another PR, feel free to refile the second commit in another PR and assign me, then I'll take a look later when I have more time. Rust Week planning is taking a lot. Good night!

To get rid of the `Ident::empty` uses.

This requires introducing `PathParser::word_sym`, as an alternative to
`PathParser::word`.
By using `@` patterns more.

Also, use `Symbol` more in a couple of errors to avoid some unnecessary
conversions to strings. This even removes a lifetime.
@nnethercote
Copy link
Contributor Author

I have removed the original second commit with the API changes; we can try again with those in a follow-up PR.

I have added a new second commit that avoids the unwraps and does a couple of other small related improvements.

@rustbot ready

@jdonszelmann
Copy link
Contributor

Nice, I like it :) And yea, I think the other changes are generally good, just can't review them properly atm because of rust week. We'll get back to it!

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented May 8, 2025

📌 Commit 603766c has been approved by jdonszelmann

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 May 8, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 8, 2025
…onszelmann

Eliminate `word_and_empty` methods.

To remove the last remaining `Ident::empty` uses.

r? ``@jdonszelmann``
bors added a commit to rust-lang-ci/rust that referenced this pull request May 8, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#140095 (Eliminate `word_and_empty` methods.)
 - rust-lang#140341 (Clarify black_box warning a bit)
 - rust-lang#140684 (Only include `dyn Trait<Assoc = ...>` associated type bounds for `Self: Sized` associated types if they are provided)
 - rust-lang#140707 (Structurally normalize in range pattern checking in HIR typeck)
 - rust-lang#140716 (Improve `-Zremap-path-scope` tests with dependency)
 - rust-lang#140800 (Make `rustdoc-tempdir-removal` run-make tests work on other platforms than linux)
 - rust-lang#140802 (Add release notes for 1.87.0)
 - rust-lang#140811 (Enable triagebot note functionality for rust-lang/rust)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6914d83 into rust-lang:master May 9, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone May 9, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 9, 2025
Rollup merge of rust-lang#140095 - nnethercote:rm-word_or_empty, r=jdonszelmann

Eliminate `word_and_empty` methods.

To remove the last remaining `Ident::empty` uses.

r? `@jdonszelmann`
@nnethercote nnethercote deleted the rm-word_or_empty branch May 9, 2025 03:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) 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.

4 participants