SE-0080 (3/5) - Failable initializers for Float->Int #3356

ultramiraculous · 2016-07-06T07:46:31Z

What's in this pull request?

Breaking out from #2742. Adds init?(exactly:) initializers for integer types from 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.

stephentyrone · 2016-07-06T19:48:43Z

stdlib/public/core/FixedPoint.swift.gyb

+    if value.isFinite == false || value < ${str(lower)}.0 || value > ${str(upper)}.0 {
+      return nil
+    }
+    self._value = Builtin.fpto${sign}i_FPIEEE${srcBits}_${BuiltinName}(value._value)


I see a check that value is in-range, but I don't see any check that the value is preserved by the conversion. Am I simply missing something? What happens if value is, e.g. 0.5?

Gah, yeah that was an oversight. I meant to change the .isFinite check to check if value % 1.0 == 0 and I somehow didn't notice the test failures last night.

FWIW, floating-point remainder is a very expensive operation. It is much cheaper to test if ${Src}(Self(value)) == value. Or once/if SE-0113 lands, it would be better still to use value.rounded(.truncate) == value.

Ah, yeah I suppose that makes sense. I think in general I'm battling a perception that converting back/forth is more expensive than it really is.

ultramiraculous · 2016-07-06T20:21:39Z

Also it looks like one of the trapping cases isn't being handled correctly in the tests w/ is causing a failure.

ultramiraculous · 2016-07-07T05:52:10Z

I think I covered everything. It turns gyb wasn't using the repr version of the floats I was trying to test, which was causing some unexpected behavior.

stephentyrone · 2016-07-07T18:55:23Z

@swift-ci Please test.

gribozavr · 2016-07-07T23:51:08Z

stdlib/public/core/FixedPoint.swift.gyb

+// Construction of integers from floating point numbers.
+%   for srcBits in allFloatBits:
+%     Src = floatName[srcBits]
+%     (lower, upper) = getFtoIBounds(srcBits, int(bits), signed)


I recommend using argument labels in the getFtoIBounds call for readability.

ultramiraculous · 2016-07-08T08:26:28Z

I think I got everything. Still running tests locally, but everything seems fine.

gribozavr · 2016-07-08T08:31:10Z

Thank you! I will look closely tomorrow, but let's get a CI run early:

@swift-ci Please test

gribozavr · 2016-07-08T21:14:14Z

@ultramiraculous The CI has errors, would you mind taking a look?

Also, I just wanted to remind you that we have a script, utils/run-test, that allows you to run just one test.

gribozavr · 2016-07-08T21:22:19Z

validation-test/stdlib/FixedPointConversion.swift.gyb

+%     else:
+
+%      if testValue > selfMax:
+FloatingPointConversionTraps.test("${Other}To${Self}Conversion/dest=${testValue}").crashOutputMatches(getTooLargeMessage()).code {


Could you wrap this line before .crashOutputMatches?

ultramiraculous · 2016-07-08T23:50:02Z

Hah, I actually didn't know about run-tests/didn't notice when it got added, so thanks! I've been hacking together that functionality on my own.

ultramiraculous · 2016-07-09T03:56:32Z

Addressed the nits and moved some helpers into a python module.

gribozavr · 2016-07-09T04:02:13Z

@swift-ci Please test

moiseev · 2016-07-29T09:35:37Z

utils/SwiftIntTypes.py

+    return (1 << bits) - 1
+
+def int_min(bits, signed):
+    return -1 * int_max(bits, signed) - 1 if signed else 0


To add some clarity, could you add parens around the first part of the expression? Otherwise it is not quite clear whether it's:

(-1 * int_max(bits, signed) - 1) if signed else 0

or

-1 * int_max(bits, signed) - (1 if signed else 0)

moiseev · 2016-07-29T13:54:10Z

FWIW: the build failure is unrelated (it's PlaygroundLogger).
Other than the code clarity nitpicks, LGTM.
/cc @gribozavr

ultramiraculous · 2016-07-29T16:04:09Z

Got the nits!

ultramiraculous · 2016-08-05T05:55:04Z

@gribozavr I just dusted off the remainder of this proposal and it's like 50 lines of code, some of which depend on this PR. Any chance you could look at this :P

lattner · 2016-08-06T03:53:02Z

Given that SE-0080 is purely additive, it is possible that we can take this. However, @tkremenek is the ultimate gatekeeper on that decision after technical review and approval has been done.

tkremenek · 2016-08-06T04:55:32Z

I would be strongly supportive of taking an implementation of SE-0080 if we feel comfortable with the implementation and it landed very soon.

gribozavr · 2016-08-09T18:51:57Z

stdlib/public/core/FixedPoint.swift.gyb

+#if !os(Windows) && (arch(i386) || arch(x86_64))
+%     end
+
+  /// Creates a ${Self} whose value is `value` rounded towards zero.


Could you restore the original doc comment?

/// Creates a new instance by rounding the given floating-point value toward /// zero. /// /// - Parameter other: A floating-point value. When `other` is rounded toward /// zero, the result must be within the range `${Self}.min...${Self}.max`.

Didn't notice it changed in the mean time. Added back.

gribozavr · 2016-08-10T01:33:37Z

Sorry for a delayed review! The changes LGTM!

@swift-ci Please test

gribozavr · 2016-08-10T08:33:40Z

SILOptimizer/copyforward.sil issue has been fixed. Retrying.

@swift-ci Please test OS X platform

ultramiraculous force-pushed the failable-float-to-int branch from a8c261a to 25bcfad Compare July 6, 2016 07:48

ultramiraculous changed the title ~~Failable float to int~~ SE-0080 (2/5) - Failable initializers for Float->Int Jul 6, 2016

ultramiraculous changed the title ~~SE-0080 (2/5) - Failable initializers for Float->Int~~ SE-0080 (3/5) - Failable initializers for Float->Int Jul 6, 2016

stephentyrone reviewed Jul 6, 2016
View reviewed changes

ultramiraculous force-pushed the failable-float-to-int branch 3 times, most recently from 168e271 to 30d18e7 Compare July 6, 2016 20:06

ultramiraculous force-pushed the failable-float-to-int branch from 30d18e7 to a449cc9 Compare July 7, 2016 05:50

gribozavr reviewed Jul 7, 2016
View reviewed changes

ultramiraculous force-pushed the failable-float-to-int branch from a449cc9 to bb5d5d8 Compare July 8, 2016 08:24

gribozavr reviewed Jul 8, 2016
View reviewed changes

ultramiraculous force-pushed the failable-float-to-int branch from 190d046 to c07bceb Compare July 9, 2016 03:55

moiseev reviewed Jul 29, 2016
View reviewed changes

ultramiraculous force-pushed the failable-float-to-int branch from 828713e to b11d14a Compare July 29, 2016 15:50

ultramiraculous added 8 commits August 1, 2016 14:27

Move Float->Int initializer to FixedPoint

1d6c3b8

SE-0080 - Failable Float->Int initializers

c3ac97d

Tests for Float->Int Initializers

ae140e6

Add max/min helpers to SwiftIntTypes

2f74e76

Create SwiftFloatingPointTypes for float-related helpers

c8458ce

Avoid truncating floats in gyb

8edc5cb

Add cFuncSuffix to SwiftFloatingPointTypes

71f6b8a

Have FloatingPointConversion use float helper methods

4caf64a

ultramiraculous force-pushed the failable-float-to-int branch from b11d14a to 4caf64a Compare August 1, 2016 21:28

tkremenek assigned gribozavr Aug 6, 2016

gribozavr reviewed Aug 9, 2016
View reviewed changes

ultramiraculous added 3 commits August 9, 2016 17:09

Restore updated FloatingPoint init documentation

bc3d616

Fix FixedPointConversion tests spacing

a8e3a17

Restore unavailable FloatingPoint abs function

636c5aa

tkremenek merged commit 420088f into swiftlang:master Aug 10, 2016

benrimmington mentioned this pull request Oct 26, 2016

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

Closed

SE-0080 (3/5) - Failable initializers for Float->Int #3356

SE-0080 (3/5) - Failable initializers for Float->Int #3356

Uh oh!

Conversation

ultramiraculous commented Jul 6, 2016

What's in this pull request?

Resolved bug number: (SR-1491)

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!

ultramiraculous commented Jul 6, 2016

Uh oh!

ultramiraculous commented Jul 7, 2016

Uh oh!

stephentyrone commented Jul 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ultramiraculous commented Jul 8, 2016

Uh oh!

gribozavr commented Jul 8, 2016

Uh oh!

gribozavr commented Jul 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ultramiraculous commented Jul 8, 2016

Uh oh!

ultramiraculous commented Jul 9, 2016

Uh oh!

gribozavr commented Jul 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moiseev commented Jul 29, 2016

Uh oh!

ultramiraculous commented Jul 29, 2016

Uh oh!

ultramiraculous commented Aug 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lattner commented Aug 6, 2016

Uh oh!

tkremenek commented Aug 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Aug 10, 2016

Uh oh!

gribozavr commented Aug 10, 2016

Uh oh!

Uh oh!

ultramiraculous commented Aug 5, 2016 •

edited

Loading