Skip to content

Conversation

Qelxiros
Copy link
Contributor

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 14, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 14, 2025

r? @petrochenkov

rustbot has assigned @petrochenkov.
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

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rustbot rustbot added the T-clippy Relevant to the Clippy team. label Sep 15, 2025
@Qelxiros
Copy link
Contributor Author

Clippy context: An aux crate (proc_macros.rs) was using Ident::to_string before comparing to a string literal, so I updated it to use the PartialEq implementation added in this PR.

@petrochenkov
Copy link
Contributor

I suggest splitting this into one PR with (insta-stable) API changes, and then another PR with implementation optimizations.

Also, PartialEq impl for Ident was previously rejected (see #78634 and its linked issue) for the reasons that are still relevant now.
Comparisons with strings are ok, Hash is probably also ok, but not especially useful without Eq.

@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 Sep 26, 2025
@Qelxiros
Copy link
Contributor Author

To clarify, the clippy change was to fix CI.

Thanks for the context! I'd be happy to remove the PartialEq<Self> impl (and Eq, by extension) or modify it to compare spans as well if you think the rest is mergeable, or leave it as is pending further discussion. I'll also link your comment on the ACP for visibility.

@Qelxiros
Copy link
Contributor Author

Qelxiros commented Oct 6, 2025

@rustbot ready

@rustbot rustbot 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 Oct 6, 2025
@petrochenkov
Copy link
Contributor

and then another PR with implementation optimizations

This was about moving the code around and providing the optimized implementation instead of just doing .to_string().
I don't think it's necessary to add the new APIs.

The Hash impl is still questionable because it's inconsistent with the only PartialEq impl, but I'll leave this for the libs team to decide.
@rustbot author

@rustbot rustbot 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 Oct 7, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 7, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

Symbol: PartialEq<str>,
T: AsRef<str> + ?Sized,
{
fn eq(&self, other: &T) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@petrochenkov I'm a little confused. Do you mean that this implementation should be reduced to self.to_string() == other.as_ref()? That seems to miss the point of the ACP, which was to compare Idents with Strings (and friends) without an allocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean that this implementation should be reduced to self.to_string() == other.as_ref()?

Yes.
Allocation or not is an implementation detail, that can be changed at any later point without any process or team decisions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you push the changes?
I still see all the unnecessary code moving and optimizations.

@Qelxiros
Copy link
Contributor Author

Qelxiros commented Oct 7, 2025

@rustbot ready

@rustbot rustbot 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 Oct 7, 2025
@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 Oct 8, 2025
@bors
Copy link
Collaborator

bors commented Oct 8, 2025

☔ The latest upstream changes (presumably #147475) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-clippy Relevant to the Clippy team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants