Fix Ord, Eq and Hash implementation of panic::Location #144510
Conversation
|
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
rustbot
added
S-waiting-on-review
| #[stable(feature = "panic_hooks", since = "1.10.0")] | ||
| impl PartialEq for Location<'_> { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.file() == other.file() && self.line == other.line && self.col == other.col |
There was a problem hiding this comment.
Is it worth flipping the order here? Comparing the filename is expensive, but in 99% of cases the line + column will mismatch on different locations I'd guess.
There was a problem hiding this comment.
I think that's fair enough for equality. What about the ordering? You could apply the same efficiency argument there although I think people would expect the ordering to be the filename > line > col.
EDIT: after double-checking the documentation, it very clearly states filename > line > col for both ordering and equality. However since that order isn't observable for equality I'll change this for efficiency.
| self.col.hash(state); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you add some test coverage for these? That would help prevent future accidental regressions.
There was a problem hiding this comment.
I'm actually not 100% sure if I can? In what little tests I've written for the standard library it's been single-file-per-test.
There was a problem hiding this comment.
You should be able to easily add some tests into library/coretests somewhere, I believe. Although you will need to have some sort of perma-unstable method to construct these that's marked #[doc(hidden)], since these are private fields.
Fixes #144486.
Now properly compares/hashes the filename rather than the pointer to the string.