[stdlib] Clean up DoubleWidth implementation #13548
Conversation
|
/cc @natecook1000 I made some additional changes to Also, it'd be nice to see if it passes existing tests (I'm traveling and can't run them on a machine of my own right now). |
|
@swift-ci Please smoke test |
|
Hmm, I will need to look into the failure. Will revise soon. |
There was a problem hiding this comment.
Is there a possibility of lowInT being negative here?
There was a problem hiding this comment.
No, because source.bitWidth > Base.bitWidth, and lowInT is the result of source masked by T(~0 as Low), which necessarily clears any sign bit.
There was a problem hiding this comment.
Can we add a test for this case?
There was a problem hiding this comment.
Yup, as mentioned, this PR is missing any tests, and I intend to cover all changes thoroughly with new tests before this is merged.
There was a problem hiding this comment.
I agree that this change is better, but the performance can degrade pretty quickly since division is slow, esp. if DoubleWidths are nested. What would you think about using hexadecimal instead?
There was a problem hiding this comment.
Is performance really a bottleneck? I think the ‘usual’ printing is very important and would argue that the performance of printing DoubleWidth<DoubleWidth<DoubleWidth<Int64>>> is not the right thing to optimize for here.
There was a problem hiding this comment.
That sounds right. If somebody does run into trouble they can just use String(x, radix: 16) instead.
|
Looking good after you chase down that FP conversion—just a couple questions for you! |
There was a problem hiding this comment.
This seems overdone to me. It looks like _combineHashValues by itself is all we'd need here.
There was a problem hiding this comment.
Sure is, but it’s what’s done for synthesized conformances. Given that Tony’s goal is eventually to allow synthesis for types with tuple members, I think we would do well to line things up here so that this extension could potentially be seamlessly deleted in the future.
|
Tests added, FP bug squashed. If you feel strongly I'll roll back the 🎄 |
|
@natecook1000 Ping. |
|
@swift-ci Please smoke test |
| lhs._storage.low |= Low(truncatingIfNeeded: highInLow) | ||
|
|
||
| lhs._storage.high >>= High(truncatingIfNeeded: rhs._storage.low) | ||
| lhs._storage.high >>= High(rhs._storage.low) |
There was a problem hiding this comment.
II think, if High is signed, this conversion can trap.
There was a problem hiding this comment.
Performing a bit cast and obtaining a negative number on overflow would be very wrong in any case, but here no overflow is possible. It cannot occur (for any type where Base.bitWidth >= 4) because the value of rhs can be at most DoubleWidth.bitWidth - 1.
It's the same conclusion reached by the original author here.
There was a problem hiding this comment.
...which is not to say that the implementation couldn't be improved. I've just taken the opportunity to streamline the logic and use masking shifts instead.
|
@swift-ci Please smoke test and merge |
|
@moiseev Well, that last commit certainly wasn't an 'improvement'--I will fix tonight. |
|
@moiseev Embarrassingly, I was calculating |
|
@swift-ci Please smoke test and merge |
|
Once more, I think. CI seems to have ignored the last request. |
|
@swift-ci Please smoke test and merge |
4 participants
This PR makes various and sundry changes to clean up and/or fix the implementation of
DoubleWidth. Most prominently:It modifies
descriptionto print the represented integer, consistent with the behavior of other built-in numeric types, and relegates output of high and low parts todebugDescription.It swaps the order in which high and low parts are evaluated for equality so that short-circuiting can happen more frequently (i.e., when the high parts are both zero).
It segregates
hashValueinto its own extension that declares conformance toHashable, consistent with earlier refactoring done onDoubleWidth.It simplifies the implementation of
magnitudeand removes a branch.It makes a few changes to the implementation of
init?<T : BinaryInteger>(exactly: T)for the sake of clarity to the reader. For instance, although we can rely on the fact thatHigh.bitWidth == Low.bitWidth, the high part is conceptually what's left after shiftingsourceright byLow.bitWidth.It deletes an entire extension for converting from binary floating-point values, since the generic methods are now implemented in [stdlib] Implement fixed-width integer conversion from binary floating point #11259 without the constraints required here.
It fixes a bug in
remainderReportingOverflow, because the remainder after dividing.minby-1is zero.It refines the implementation of masking shifts.