Repository navigation
Conversation
|
rustbot has assigned @petrochenkov. Use |
55be783 to
fee6f71
Compare
This comment has been minimized.
This comment has been minimized.
fee6f71 to
624c863
Compare
This comment has been minimized.
This comment has been minimized.
624c863 to
509f080
Compare
This comment has been minimized.
This comment has been minimized.
509f080 to
8bc4c57
Compare
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
Clippy context: An aux crate (proc_macros.rs) was using |
|
I suggest splitting this into one PR with (insta-stable) API changes, and then another PR with implementation optimizations. Also, |
|
To clarify, the clippy change was to fix CI. Thanks for the context! I'd be happy to remove the |
|
@rustbot ready |
This was about moving the code around and providing the optimized implementation instead of just doing The |
|
Reminder, once the PR becomes ready for a review, use |
| Symbol: PartialEq<str>, | ||
| T: AsRef<str> + ?Sized, | ||
| { | ||
| fn eq(&self, other: &T) -> bool { |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Did you push the changes?
I still see all the unnecessary code moving and optimizations.
|
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Looks like there was some labeling confusion above but as I understand it, this needs a test fixed then it's ready for team discussion again. It would be good to exercise these in a test to make sure we don't regress exact behavior here, e.g. w.r.t. raw identifiers. |
|
Based upon my understanding this is okay to FCP but would need a rebase. Would you be willing to update this @Qelxiros? r? libs |
266a6fb to
836f058
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
The job Click to see the possible cause of the failure (guessed by this bot)Important For more information how to resolve CI failures of this job, visit this link. |
|
☔ The latest upstream changes made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
View all comments
ACP: rust-lang/libs-team#573