Fix sbyte arrays comparison #5267
Conversation
|
I'm redoing this code, but anyway... |
|
@manofstick ah, ok. While you are on it, what about adding a special case for |
|
This PR should probably be closed, as being addressed in #5278 |
|
@manofstick yes, as soon as your PR is merged, which seems not going to happen soon :) |
|
Agree that #5278 won't be merged anytime soon! (It's been three years since the original PR appeared! Although I don't expect another three years to pass...) But I'm not suggesting that the original issue #5263 be closed, just rather this PR as it will also satisfied by #5278. The problem has been in FSharp.Core for years, and no one has complained (I'm trying to imagine a case where you would be using an array of sbytes in a comparison type situation... I'm sure there is, but it's not a common occurrence!) Anyway, just talking from personal experience of managing issues/merges that the less of them there are the better! (and once they reach a critical amount then they just get forgotten about...) But hey, I'm not the one who has to manage them, so really up to whoever is... |
|
Given that this is explicitly marked as "fast path", I wonder if this is intentional, as per the blog post mentioned in the issue: #5263. If so, then this fix would have to be considered a breaking change. I do wonder why anyone would rely on this working though. |
|
Welcome Phillip! I agree that this is a "breaking change", but I doubt it was even intentional. I think it is just a corner case which was never used. The failing tests that are part of this PR where ones I put in place the last time I attempted to modify comparers where I created a huge regression set, just for this particular reason (well I wasn't hoping to find a bug in existing code, but you know what I mean!). In #5278 I have "fixed" If you want to leave it as is (which I think is the worse option) then I can re-implement this bug in #5278... |
|
Closing per: @manofstick issue: #5263 will be managed by : #5278 |
This fixes #5263