-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Implement DepTrackingHash for Option
through blanket impls instead of macros
#84234
Conversation
Somewhat unrelated to this PR - I think defining the order-independence of the hash at the type level is the wrong way to go about this. Nothing is preventing the rest of the compiler from relying on the option order. Instead, I think |
So I'm looking back over this - I think the only way this could change a hash is that
Having |
That makes sense to me. I see the following
Are llvm's arguments order-sensitive? Or |
Ok, I take it back, I see the following order-dependent uses:
and indirectly through
There might be more, I only looked at Should I just sort all these and hope nothing breaks? |
This comment has been minimized.
This comment has been minimized.
The line in I'm not sure about |
…nkov Reduce duplication in `impl_dep_tracking_hash` macros Cherry-picked from rust-lang#84234 since it will be a while until it lands.
…nkov Reduce duplication in `impl_dep_tracking_hash` macros Cherry-picked from rust-lang#84234 since it will be a while until it lands.
…nkov Reduce duplication in `impl_dep_tracking_hash` macros Cherry-picked from rust-lang#84234 since it will be a while until it lands.
…nkov Reduce duplication in `impl_dep_tracking_hash` macros Cherry-picked from rust-lang#84234 since it will be a while until it lands.
…nkov Reduce duplication in `impl_dep_tracking_hash` macros Cherry-picked from rust-lang#84234 since it will be a while until it lands.
…nkov Reduce duplication in `impl_dep_tracking_hash` macros Cherry-picked from rust-lang#84234 since it will be a while until it lands.
I won't have time to follow up on this in the next month or so. Do you think this is ok to land as-is or do you want the broader change with sorting first? |
…nkov Reduce duplication in `impl_dep_tracking_hash` macros Cherry-picked from rust-lang#84234 since it will be a while until it lands.
…nkov Reduce duplication in `impl_dep_tracking_hash` macros Cherry-picked from rust-lang#84234 since it will be a while until it lands.
…nkov Reduce duplication in `impl_dep_tracking_hash` macros Cherry-picked from rust-lang#84234 since it will be a while until it lands.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…of macros This avoids having to add a new macro call for both the `Option` and the type itself.
Option
through blanket impls instead of macros
Ok, this is a nice small PR now. |
@bors r+ |
📌 Commit 76502de has been approved by |
☀️ Test successful - checks-actions |
This avoids having to add a new macro call for both the
Option
and the type itself.Noticed this while working on #84233.
r? @Aaron1011