refactor svh #26629
refactor svh #26629
Conversation
|
r? @Aatch (rust_highfive has picked a reviewer for you, use r? to override) |
I have vague concerns about how this allocates on attempts to look at it's representation as a str, however it doesn't appear that this is a common case in tight loops.
Downstream consumers of Svh now use the ToString and Hash traits whenever possible
| hash.to_string() | ||
| } else { | ||
| hash.chars().rev().collect() | ||
| }; |
There was a problem hiding this comment.
This seems somewhat odd, and I don't understand how endianness helps here, so could you clarify what this is doing?
There was a problem hiding this comment.
In the to_string() impl, it walks the low nibble to the high nibble constructing the string, the net result being that without this, Svh::new("deadbeefdeadbeef").to_string() returns feebdaedfeebdaed
Unfortunately, because it's flipped nibble wise not byte wise I can't just flip the bytes in the resulting u64, but on a big endian architecture this reversing shouldn't be necessary.
There was a problem hiding this comment.
Yeah I understand that this is reversing the string, but what I don't understand is why it matters to have the same ordering across different architectures?
There was a problem hiding this comment.
Oh right. It needs to maintain compatibility with what's emitted today to avoid breaking the abi (without flipping it on LE arches it will refuse to build a stage1, because when it goes to consume the libc and core crates it'll believe they're out of date). I believe it's safe to nop out on BE architectures since they're already in the right order.
There was a problem hiding this comment.
We don't currently maintain ABI compatibility today, so that's not too much of a worry, and I feel like if we cross endianness platforms that this is likely to be the least of our problems (I'm also not 100% sure that this is a problem). Altogether I think it's fine to omit this.
|
Out of curiosity, what's the motivation for switching to a u64 instead of a string? I don't really see a compelling argument either way, I'm just curious. |
|
The underlying idea was to be able to hash Attr's in a way that ignore'd the insides of their spans for detecting dups, and constructing Strings inside of other parts of rustc seemed not obviously ideal, although after digging through this a little and considering some of the implementation quirks it might be worth winding this back a bit. |
|
Looking at this, I can easily split out deduping the hex impl and using ToString, do you want that in a separate PR (that I think is Definitely Good), while we discuss the rest? |
|
I think this change is fine to be here, I just still haven't quite gotten to the point where I understand what it's targeted at doing. |
|
☔ The latest upstream changes (presumably #27176) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@alexcrichton what's the status here? |
|
I think there were some changes that @richo planned, but feel free to reopen if those are made! |
This is an excercise in yak shaving, started because I wanted to implement a lint for duplicated attrs, which lead me into the Svh machinery.
Unfortunately, due to this comment I suspect it's still not possible, but this simplifies the interfaces and internal representations of Svh.
(Opening the PR and then immediately rebasing because it won't merge, but I think github might nuke my comment)