-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
[WIP] Accommodate exceptional values for array comparison #12503
Conversation
@airspeedswift I know you don't like |
What about the Decimal in Foundation? Does the same logic apply there? |
@swift-ci Please smoke test |
@swift-ci Please benchmark |
@moiseev Yes, applies to |
Build comment file:Optimized (O)Regression (12)
Improvement (20)
No Changes (302)
Unoptimized (Onone)Regression (6)
Improvement (11)
No Changes (317)
Hardware Overview
|
This is a neat solution for this one case, but it's a sticking plaster over the fundamental issue. More generally, ignoring the requirements of With conditional conformance, we are going to be able to make
For these reasons, I'm inclined to not take this particular fix until it's finalized that the underlying problems with FP comparison aren't going to be fixed. Otherwise, we are just perpetuating the issue. |
@airspeedswift I understand the desire to solve this definitively. This PR, however, is a strict improvement over the status quo and—as I have argued—essentially orthogonal to the floating point issue in the sense that consistent behavior for Array.== can be accommodated regardless of the behavior of FloatingPoint.==. Believe it or not, a similarly terse fix exists for all generic algorithms vended by the standard library. This is one of a series of incremental steps that can indeed fix the issue of Set deduping and sort corruption. The conditional conformance of Array to Equatable is accommodated by having _containsExceptionalValues return |
Yes, we can fix issues within the Standard Library on a case-by-case basis by adding underscored methods. The std lib is privileged in that it can add dynamically dispatched customization points as needed. But the algorithms in the std lib shouldn't be special, user code can get bitten by this just as easily. Also, bear in mind that as we venture into ABI-stable territory, these custom workarounds become permanent and less able to be backed out in future versions. |
We should probably have this discussion on swift-dev rather than the PR... |
Agree. Let's move the overarching discussion to swift-dev. Suffice it to say, though, that the intention is not to leave these underscored permanently. Either |
Array.==
tests for referential equality and returnstrue
if the underlying buffer is shared between two instances. This provides for unexpected behavior in the case of comparisons involving floating-point NaN. SinceDouble.nan != Double.nan
, the result of a comparison of arrays that contain NaN would depend on whether the underlying buffer is shared.Whether
Equatable
ought to accommodate IEEE-mandated behavior of floating-point NaN is potentially a design topic to be revisited, but the inconsistent behavior here requires a fix in order to permit predictable behavior in the face of the current, deliberate implementation ofFloatingPoint.==
.In documentation for
Comparable
, a note states:This PR incorporates this particular exception into an underscored static protocol requirement of
Equatable
named_containsExceptionalValues
. A default implementation returnsfalse
and floating-point types returntrue
.Array.==
then consults this static requirement prior to relying on referential equality.Resolves SR-6181.