SE-0080 (2/5) - Failable initializers for Float->Float #2977

ultramiraculous · 2016-06-10T07:33:40Z

What's in this pull request?

Breaking out from #2742. Adds init?(exactly:) initializers for float types from other float types.

Resolved bug number: (SR-1491)

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

gribozavr · 2016-06-22T00:55:56Z

@swift-ci Please test

gribozavr · 2016-06-22T23:30:11Z

stdlib/public/core/FloatingPointTypes.swift.gyb

 #if !os(Windows) && (arch(i386) || arch(x86_64))
-%     end
+%   end

  /// Construct an instance that approximates `other`.


Please restore the old comment for the case when Self is That (Create an instance initialized to ...).

gribozavr · 2016-06-23T00:02:42Z

@stephentyrone Could you review, please?

I'm not 100% sure what is the desired behavior for NaN. I can see it argued both ways:

Since NaNs are not equal to each other, this initializer can't return any value that can possibly round-trip and compare equal to the original value. So FloatXX(exactly: NaN) should return nil.
Conversions between floating point formats should propagate NaNs if the target format supports them, so FloatXX(exactly: NaN) should return a NaN.

gribozavr · 2016-06-23T00:40:45Z

validation-test/stdlib/FloatingPointConversion.swift.gyb

+#if arch(i386) || arch(x86_64)
+%     end
+
+%   for OtherFloat, otherSignificandBits in floatNameToSignificandBits.iteritems():


Indentation of the second-level %if and %for is inconsistent, would you mind fixing it?

Think I got this right. You're right, it was all over the place from moving things around over time.

ultramiraculous · 2016-06-23T03:32:43Z

Yeah, so @gribozavr / @stephentyrone on the NaN deal: I went back and forth on it and meant to leave a note asking for guidance. It seemed like not propagating the NaN is the right move, but I got stuck going back and forth on it. Maybe it should propagate, like the -Inf/+Inf special cases? It's at the very least not the same case as truncation, where we're advertising that we'll return nil.

ultramiraculous · 2016-06-23T03:34:55Z

(Also I guess the change I just made to srcBits <= bits is about to cause my local test run to fail, because the NaN case isn't going to be handled the same in every case...)

stephentyrone · 2016-06-24T15:28:43Z

There are two competing considerations WRT how NaN is handled:

IEEE 754: "For an operation with quiet NaN inputs, other than maximum and minimum operations, if a floating-point result is to be delivered the result shall be a quiet NaN"
We would like the have the axiom that either result == source or result is nil.

General practice in floating-point is that it's OK to "lose" NaNs if the exceptional condition is tracked by some other means (here, returning nil). So formally, I think we can justify either behavior. There's not a huge performance difference either way (though preserving NaN will be slightly faster for concrete types on x86).

If we preserve NaNs, we preserve some information about what went wrong, which is usually good.
If we return nil for NaNs, we make it more likely that an exceptional case will be detected and handled promptly, which is also good.

I come down weakly in favor of returning nil for NaN, as I believe that it's slightly friendlier to inexperienced users, and experts can always get the other behavior with a one-line workaround.

stephentyrone · 2016-06-24T15:33:22Z

stdlib/public/core/FloatingPointTypes.swift.gyb

+    if ${That}(self) != other {
+      return nil
+    }
+%   endif


endif vs end?

Yeah I've got that fixed locally. I was waiting on ideas on NaN handling before I pushed changes again.

ultramiraculous · 2016-06-24T23:27:16Z

stdlib/public/core/FloatingPointTypes.swift.gyb

+  /// Create an ${That} initialized to `value`, if `value` can be represented without rounding.
+  /// - note: This is effectively checking that `inputValue == outputValue`, meaning `NaN` inputs will always return `nil`. 
+  @_transparent
+  public init?(exactly other: ${That}) {


Does this look ok as a revision?

Could you change the function to be @inline(__always) instead of @_transparent?

To elaborate, @_transparent affects compile-time diagnostics, and I don't think we have any special diagnostics support in the compiler for this new initializer.

gribozavr · 2016-06-25T00:51:26Z

@ultramiraculous Thank you! LGTM modulo comments. Let's run it in CI:

@swift-ci Please test

gribozavr · 2016-06-29T18:05:39Z

@swift-ci Please test

gribozavr · 2016-06-29T18:35:14Z

stdlib/public/core/FloatingPointTypes.swift.gyb

+  }
+
+  /// Create an ${That} initialized to `value`, if `value` can be represented without rounding.
+  /// - note: This is effectively checking that `inputValue == outputValue`, meaning `NaN` inputs will always return `nil`. 


Should "an ${That}" be "a ${That}"? The substitutions are Float, Double, and Float80.

Also, could you wrap to 80 columns and add an empty line before "- note"?

gribozavr · 2016-06-29T18:51:42Z

@ultramiraculous Thank you very much for working on this PR!

gribozavr · 2016-06-30T00:56:25Z

@swift-ci Please test and merge

ultramiraculous closed this Jun 10, 2016

ultramiraculous reopened this Jun 10, 2016

ultramiraculous changed the title ~~Failable float float~~ SE-0067 (2/5) - Failable initializers for Float->Float Jun 10, 2016

ultramiraculous force-pushed the failable-float-float branch from e16ffba to e2b9c14 Compare June 14, 2016 04:36

gribozavr reviewed Jun 22, 2016
View reviewed changes

gribozavr reviewed Jun 23, 2016
View reviewed changes

ultramiraculous force-pushed the failable-float-float branch from e2b9c14 to 9345d65 Compare June 23, 2016 03:23

stephentyrone reviewed Jun 24, 2016
View reviewed changes

ultramiraculous force-pushed the failable-float-float branch from 9345d65 to 548911f Compare June 24, 2016 23:26

ultramiraculous reviewed Jun 24, 2016
View reviewed changes

ultramiraculous force-pushed the failable-float-float branch from 548911f to c0f7a4e Compare June 26, 2016 04:08

gribozavr reviewed Jun 29, 2016
View reviewed changes

ultramiraculous force-pushed the failable-float-float branch 2 times, most recently from d5ed380 to e7a5e67 Compare June 29, 2016 22:30

ultramiraculous changed the title ~~SE-0067 (2/5) - Failable initializers for Float->Float~~ SE-0080 (2/5) - Failable initializers for Float->Float Jun 29, 2016

ultramiraculous added 2 commits June 29, 2016 16:23

SE-0080 - Failable initializers for Float->Float

26623fd

Float->Float Failable Initializer Tests

025a699

ultramiraculous force-pushed the failable-float-float branch from e7a5e67 to 025a699 Compare June 29, 2016 23:23

swift-ci merged commit 8fe85c3 into swiftlang:master Jun 30, 2016

benrimmington mentioned this pull request Oct 26, 2016

[SR-1491] Implement support for SE-0080: Failable Numeric Conversion Initializers #44100

Closed

SE-0080 (2/5) - Failable initializers for Float->Float #2977

SE-0080 (2/5) - Failable initializers for Float->Float #2977

Uh oh!

Conversation

ultramiraculous commented Jun 10, 2016

What's in this pull request?

Resolved bug number: (SR-1491)

Uh oh!

gribozavr commented Jun 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Jun 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ultramiraculous commented Jun 23, 2016

Uh oh!

ultramiraculous commented Jun 23, 2016

Uh oh!

stephentyrone commented Jun 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Jun 25, 2016

Uh oh!

gribozavr commented Jun 29, 2016

Uh oh!

gribozavr Jun 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Jun 29, 2016

Uh oh!

gribozavr commented Jun 30, 2016

Uh oh!

Uh oh!

gribozavr Jun 29, 2016 •

edited

Loading