-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Derive Eq and Hash for SourceInfo again #65828
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
It is somewhat concerning to me that we've hit this sort of PR a couple times in the last few days -- I don't have links just now -- I wonder if it's worth at least adding a comment on that derive saying, e.g., "Cranelift, at least as of #65828, is also using these impls so ping @bjorn3 if removing them" I think I would personally prefer that we instead say something like "Cranelift et al" should wrap our types and we should make that easy if they need these impls, but I understand that Rust makes that quite hard so I don't think we can just do that. r=me with a comment that lists the derives that are needed for cranelift (like I suggested in my first paragraph) |
@bors rollup |
12c1b46
to
f04867c
Compare
Amended with comment. I also checked if |
@bors r+ |
📌 Commit f04867c has been approved by |
⌛ Testing commit f04867c with merge e5740fa9cb12d4c8d6769944f0343bb30a9f948a... |
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - checks-azure |
Spurious network error. |
Indeed, it's a network error. |
…etrochenkov Derive Eq and Hash for SourceInfo again In https://github.com/bjorn3/rustc_codegen_cranelift/blob/75c24b9c9677600422ec86fa9f4c78fe3678d2ce/src/common.rs#L368 I store it in a `indexmap::IndexSet`, which requires `Eq` and `Hash`. Unfortunately they were removed in rust-lang#65647, so I can't update to latest nightly.
Rollup of 8 pull requests Successful merges: - #65743 (rustc_typeck: don't record direct callees in generator_interior.) - #65761 (libsyntax: Enhance documentation of the AST module) - #65772 (Remove the last remaining READMEs) - #65773 (Increase spacing for suggestions in diagnostics) - #65791 (Adding doc on keyword continue) - #65824 (rustc: make DefPathData (and friends) Copy (now that it uses Symbol).) - #65828 (Derive Eq and Hash for SourceInfo again) - #65842 (Add more information on rustdoc search) Failed merges: - #65825 (rustc: use IndexVec<DefIndex, T> instead of Vec<T>.) r? @ghost
@@ -467,7 +467,9 @@ impl<T: Decodable> rustc_serialize::UseSpecializedDecodable for ClearCrossCrate< | |||
/// Grouped information about the source code origin of a MIR entity. | |||
/// Intended to be inspected by diagnostics and debuginfo. | |||
/// Most passes can work with it as a whole, within a single function. | |||
#[derive(Copy, Clone, Debug, PartialEq, RustcEncodable, RustcDecodable, HashStable)] | |||
// The unoffical Cranelift backend, at least as of #65828, needs `SourceInfo` to implement `Eq` and | |||
// `Hash`. Please ping @bjorn3 if removing them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't find this kind of comment particularly useful. Also, I would expect you'd use an IndexVec<SourceScope, ...>
instead of working with SourceInfo
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment was suggested at #65828 (comment). I am getting the SourceInfo
directly from Statement
and Terminator
, but I need to get an unique u32 to pass through cranelift
as SourceLoc
, while being able to get the original SourceInfo
back after having compiled a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, unless you are emitting variable debuginfo somehow, you'd only need the Span
, not the whole SourceInfo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, unless you are emitting variable debuginfo somehow
Not yet
you'd only need the Span, not the whole SourceInfo.
Indeed only Span
is used currently, but I will probably need the rest of the SourceInfo
.
In https://github.com/bjorn3/rustc_codegen_cranelift/blob/75c24b9c9677600422ec86fa9f4c78fe3678d2ce/src/common.rs#L368 I store it in a
indexmap::IndexSet
, which requiresEq
andHash
. Unfortunately they were removed in #65647, so I can't update to latest nightly.