Skip to content
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

StringSegment GetHashCode() includes the Offset but Equals doesn't care about it #137

Closed
IanMercer opened this issue Aug 3, 2016 · 7 comments
Assignees

Comments

@IanMercer
Copy link

Surely GetHashCode() should not rely on the Offset since in Equals, two StringSegments are equal if they have equal values regardless as to where they are located in the string.

@pranavkm
Copy link

pranavkm commented Aug 3, 2016

Yup, seems like a bug. It should probably add individual chars to the HashCodeCombiner to compute the hash.

@IanMercer
Copy link
Author

I think if you just delete these two lines from GetHashCode it's all good:

        hash.Add(Offset);
        hash.Add(Length);

But yes, if you can do it without an allocation that might be better.

@pranavkm
Copy link

pranavkm commented Aug 3, 2016

Wouldn't that still be problematic if the two strings are different but the StringSegments are identical.

@IanMercer
Copy link
Author

Value is Buffer.Substring(Offset, Length)

So either you add Value to the Hashcode (which is effectively what Equals is looking at), or you add the characters from Buffer from position Offset for Length characters.

@pranavkm
Copy link

pranavkm commented Aug 3, 2016

Huh. I thought it was adding the Buffer to the combiner. Allocating as part of GetHashCode seems counter to the purpose of this type. cc @rynowak.

@IanMercer
Copy link
Author

Interestingly ArraySegment has different (and arguably rather useless) semantics for Equals: it does a compare on the whole buffer and the length and the offset. When would that ever be useful? Since StringSegment and ArraySegment are convenient ways of slicing strings and arrays to save copying you'd expect Equals to work like it would if you had copied the segment out, i.e. do the characters match (or do the array elements match).

@Eilon Eilon added bug labels Aug 4, 2016
@Eilon Eilon added this to the 1.1.0 milestone Aug 4, 2016
@Eilon
Copy link
Member

Eilon commented Aug 4, 2016

@pranavkm please confirm with @rynowak what the expectation is here.

pranavkm added a commit that referenced this issue Aug 4, 2016
pranavkm added a commit that referenced this issue Aug 4, 2016
@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants