Spans now derive equality #31277
Spans now derive equality #31277
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Why is this needed if derived equality is never used (but now can be used accidentally)? |
|
Spans derive hashing, and are used in HashSets - I felt it strange to derive hashing and not derive equality. |
| } | ||
|
|
||
| pub fn contains(self, other: Span) -> bool { | ||
| self.lo <= other.lo && other.hi <= self.hi | ||
| } | ||
|
|
||
| pub fn source_equal(&self, other: &Span) -> bool { | ||
| (*self).lo == (*other).lo && (*self).hi == (*other).hi |
There was a problem hiding this comment.
The dereferencing is unnecessary. Just let autoderef do its job
I can't check right now, but this usually means that hashes are incorrect and the hash sets work by chance. |
|
This comment/code should probably be updated as well |
|
Good catch, I'll update that in the morning. |
|
@petrochenkov yes, without this PR hashing for spans is probably buggy. The point of this PR is to fix that so that |
| } | ||
|
|
||
| pub fn contains(self, other: Span) -> bool { | ||
| self.lo <= other.lo && other.hi <= self.hi | ||
| } | ||
|
|
||
| pub fn source_equal(&self, other: &Span) -> bool { |
There was a problem hiding this comment.
Could you add a comment stating when to use this vs when to use ==
DanielJCampbell
force-pushed
the
SpanEquality
branch
from
b872c05 to
e1be504
Compare
|
Suggested changes made |
|
@bors: r+ |
|
📌 Commit e1be504 has been approved by |
r? @nrc