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

Add the DoubleWidth type #197

Merged
merged 1 commit into from
Oct 27, 2021
Merged

Add the DoubleWidth type #197

merged 1 commit into from
Oct 27, 2021

Conversation

benrimmington
Copy link
Contributor

@benrimmington benrimmington commented Aug 31, 2021

The DoubleWidth<Base> type has twice the bit width of its base type.
It was proposed in SE-0104, but never shipped (due to compiler problems).

Start date End date File history
2017-01-05 2017-06-05 stdlib/public/core/Integers.swift.gyb
2017-06-06 2018-03-20 stdlib/public/core/DoubleWidth.swift.gyb
2018-03-23 2019-03-01 test/Prototypes/DoubleWidth.swift.gyb

The DoubleWidth.swift file is generated from the latest GYB template:

utils/gyb --line-directive="" test/Prototypes/DoubleWidth.swift.gyb
arch config compilation
x86_64 debug 1.2 seconds
x86_64 release 2.5 seconds
arm64 release 2.4 seconds

The DoubleWidthTests.swift file is a translation of the original tests (from StdlibUnittest to XCTest), excluding the expectCrashLater() calls.

arch config compilation
x86_64 debug 5.8 seconds
x86_64 release 9.7 seconds
arm64 release 9.6 seconds

There are also some new and updated tests — for string conversions and SR-6947.

arch config all / slowest tests execution
x86_64 debug all DoubleWidthTests 2.0 seconds
x86_64 debug …_Hexadecimal() 1.2 seconds
x86_64 debug …_LeftAndRightShifts() 0.7 seconds
x86_64 release all DoubleWidthTests 1.2 seconds
x86_64 release …_Hexadecimal() 0.7 seconds
x86_64 release …_LeftAndRightShifts() 0.4 seconds

@benrimmington
Copy link
Contributor Author

I tried to test the DoubleWidth: _ExpressibleByBuiltinIntegerLiteral conformance:

+      swiftSettings: [.unsafeFlags(["-parse-stdlib"])]

+import Swift

-  public init(_builtinIntegerLiteral _x: Builtin.IntegerLiteral) {
+  public init(_builtinIntegerLiteral _x: Builtin.IntLiteral) {

-    if !Bool(_overflow) {
+    if !Bool(_builtinBooleanLiteral: _overflow) {

-  public init(integerLiteral x: Int) {
-    self.init(x)
-  }

There are two unrecognized builtins:

  • Builtin.sext_Int64_IntLiteral
  • Builtin.ashr_IntLiteral

When arbitrary-precision literals were implemented, DoubleWidth: _ExpressibleByBuiltinIntegerLiteral was already within the #if false block:

https://github.com/apple/swift/pull/20208/files?file-filters%5B%5D=.gyb#diff-11720e84c8f7b8f4dfedfb52e97e620ec4d9249f662995d7fc67c6ce96c0438f

@stephentyrone
Copy link
Member

Hi @benrimmington thanks. Just to set expectations, I'm booked up this week and on vacation next week, so it will probably be a little while before I get to take a look at this in any detail.

@stephentyrone
Copy link
Member

stephentyrone commented Oct 26, 2021

Hi Ben, finally getting a chance to look at this, sorry for the delay.

Generally this seems fine (as expected, given that it's mostly borrowed from Swift's prototype). I think the only thing I'd like to tweak is a meta-point of not packaging a library for it yet (since it's still "experimental" and hence we don't really want to encourage use outside the project). However, I do have fairly immediate use cases in mind for testing some of the new IntegerUtilities API, so I think the best thing to do would be to move DoubleWidth into TestUtilities, where it will be available for use testing anything else.

If you have a specific use case that merits vending a library, let's discuss further.

@benrimmington
Copy link
Contributor Author

move DoubleWidth into TestUtilities

Do you want me to do this, or would it be better if you (or Nate Cook, as the original implementer of DoubleWidth) added the files? TestUtilities would be a new module, I assume, unless you're referring to _TestSupport — but that seems to be only for real and complex testing?

@stephentyrone
Copy link
Member

stephentyrone commented Oct 26, 2021

Sorry, yes, I mean _TestSupport. I'm happy to do this if you don't have time at the moment.

@benrimmington benrimmington marked this pull request as draft October 26, 2021 17:13
@stephentyrone
Copy link
Member

Thanks! I'll finish reviewing this later today or tomorrow.

@stephentyrone
Copy link
Member

@swift-ci please test

@benrimmington
Copy link
Contributor Author

I moved DoubleWidthTests.swift into the IntegerUtilitiesTests target. (Alternatively, _TestSupport could have its own test target.) The CMake build might be broken at the moment, but I don't know how to test this.

@stephentyrone
Copy link
Member

I'll take a look at the CMake build as part of my review.

@stephentyrone stephentyrone self-requested a review October 27, 2021 16:17
stephentyrone
stephentyrone previously approved these changes Oct 27, 2021
@benrimmington
Copy link
Contributor Author

IntegerUtilitiesTests now has a _TestSupport dependency (in Package.swift), shall I update CMakeLists.txt as well?

Co-authored-by: Nate Cook <[email protected]>
Co-authored-by: Max Moiseev <[email protected]>
Co-authored-by: Xiaodi Wu <[email protected]>
@stephentyrone
Copy link
Member

@swift-ci test

@stephentyrone stephentyrone merged commit b501ca4 into apple:main Oct 27, 2021
@benrimmington benrimmington deleted the experimental branch October 27, 2021 20:05
@xwu
Copy link
Contributor

xwu commented Oct 27, 2021

Delighted to see this code in runnable form again—thanks @benrimmington!

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

Successfully merging this pull request may close these issues.

3 participants