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

ComparisonIdentity.Structural.Compare returns different values for Option wrapped types #576

Closed
manofstick opened this issue Aug 8, 2015 · 5 comments

Comments

@manofstick
Copy link
Contributor

My regression tests for #513 have shown up the following inconsistency in existing f#:

printfn "%d" <| (ComparisonIdentity.Structural.Compare (SByte.MinValue, SByte.MaxValue))
printfn "%d" <| (ComparisonIdentity.Structural.Compare (Some SByte.MinValue, Some SByte.MaxValue))

Output

-1
-255

It is due to FSharpOption's type generic call to:

LanguagePrimitives.HashCompare.GenericComparisonWithComparerIntrinsic<T>(genericComparer, x, y);

Now I doubt if anyway is using comparers in any other way than < 0 or > 0, but they may be? #513 is a potentially breaking change because it returns -1 for the Option case - i.e. matching the primitive operation (although the affects of this breaking case, to Option comparison operations being sensitive to particular return values - is almost vanishingly small I would say..)

(The good news is that if in my regression limits Compare to (sign of Compare) then I'm getting no regression errors. I'll put my regression test up soon)

@manofstick
Copy link
Contributor Author

Playing around a bit this seems to be the case only for primitives that are < 32 bits. Even more smaller probability that this affects real code...

@manofstick
Copy link
Contributor Author

I have just fixed #513 to regression test properly (I.e. it now acts in same way as existing configuration), but this is still an oddity of the existing configuration along with the #527 & #556 (although this is less severe than those)

@latkin
Copy link
Contributor

latkin commented Aug 10, 2015

I don't think this is worth worrying about. It's well-documented that comparison results should only be considered for 0, < 0 or > 0.

@manofstick
Copy link
Contributor Author

I agree. (For regression purposes trying to just be 100% with what is currently there so I don't have to look at any individual cases, but I would like to achieve ComparerIdentity.Structural as equal to System.Collections.Generic.Comparers.Default where they shouldn't differ - even if it is still "valid": values)

@dsyme
Copy link
Contributor

dsyme commented Aug 14, 2015

Yes, this is not a bug, will close it as by design

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants