[docs] Update and fill in documentation for DoubleWidth and integer protocols #14295

xwu · 2018-01-31T02:58:58Z

This PR contains documentation changes from PR #13784, which was split up to be committed in chunks after CI stopped cooperating.

In particular, DoubleWidth members now gain doc comments, dividedReportingOverflow and remainderReportingOverflow have their doc comments substantially updated, and several outdated comments are deleted from integer protocols.

xwu · 2018-01-31T03:00:13Z

stdlib/public/core/DoubleWidth.swift.gyb

 ///
 /// You can use the `DoubleWidth` type to continue calculations with the result
 /// of a full width arithmetic operation. Normally, when you perform a full
-/// width operation, the result is a tuple of the high and low components of
-/// the result.
+/// width operation, the result is a tuple of the high and low parts of the


In the documentation for integer protocols, "parts" seems to be the preferred term. I think that wording is superior in this case, as these aren't components in the mathematical sense.

xwu · 2018-01-31T03:01:33Z

stdlib/public/core/DoubleWidth.swift.gyb

  @_inlineable // FIXME(sil-serialize-all)
  @_transparent
-  public // @testable
-  init(_ _value: (High, Low)) {
+  public init(_ value: (high: High, low: Low)) {


Although it appears that this initializer was originally not intended for public consumption, the documentation tells users to use it. In fact, this initializer is promoted as part of the main use case for DoubleWidth (and I would agree with that characterization).

I didn't even notice that this wasn't a public initializer when I wrote that—seems like a pretty intuitive usage of the type.

@moiseev, what do you think of making this public API?

I agree we should just remove the underscore from _value and a // @testable comment. This initializer is quite useful and should be a part of DoubleWidth API.

Great. I think this PR is ready then :)

xwu · 2018-01-31T03:02:23Z

stdlib/public/core/Integers.swift.gyb

-  /// Returns the sum of this value and the given value along with a flag
-  /// indicating whether overflow occurred in the operation.
+  /// Returns the sum of this value and the given value, along with a Boolean
+  /// flag indicating whether overflow occurred in the operation.


There's some vacillation here between "flag" and "Boolean value," probably a leftover from the days of ArithmeticOverflow. For consistency, I've standardized on "Boolean flag."

Good catch on the "flag" business—these should all be "Boolean value indicating..." throughout to match the rest of the docs.

xwu · 2018-01-31T03:02:50Z

stdlib/public/core/Integers.swift.gyb

  ///   component is `false`, the `partialValue` component contains the entire
  ///   quotient. If the `overflow` component is `true`, an overflow occurred
-  ///   and the `partialValue` component contains the truncated quotient.
+  ///   and the `partialValue` component contains either the truncated quotient
+  ///   or, if the quotient is undefined, the dividend.


This, I think, is a key clarification.

xwu · 2018-01-31T03:03:16Z

stdlib/public/core/Integers.swift.gyb

 """,
        '%': """\
-  // FIXME(integers): the comment is for division instead of remainder


This issue is now fixed, and the return value is greatly clarified.

xwu · 2018-01-31T03:03:57Z

stdlib/public/core/Integers.swift.gyb

@@ -896,15 +900,15 @@ def unsafeOperationComment(operator):
  /// - Returns: The sum of this value and `rhs`.
 """,
        '-': """\
-  /// Returns the difference of this value and the given value without checking
-  /// for arithmetic overflow.
+  /// Returns the difference obtained by subtracting the given value from this


Not that there would have been terrible confusion, but we could stand to clarify which value is the subtrahend, so I did.

xwu · 2018-01-31T03:05:12Z

stdlib/public/core/Integers.swift.gyb

  ///
  /// - Parameter dividend: A tuple containing the high and low parts of a
-  ///   double-width integer. The `high` component of the value carries the
-  ///   sign, if the type is signed.


This seems like an unnecessary and potentially confusing remark; it is not found anywhere else, even though a similar tuple is returned in other functions.

The two's-complement value will differ from its magnitude in both the high and low parts, and it's highly doubtful that anyone will wonder if it's the low part and not the high part that carries the actual sign bit itself.

xwu · 2018-01-31T03:06:35Z

stdlib/public/core/Integers.swift.gyb

@@ -2555,11 +2557,8 @@ extension FixedWidthInteger {

 % for x in binaryArithmetic['Numeric'] + binaryArithmetic["BinaryInteger"][:1]:
 %   callLabel = x.firstArg + ': ' if not x.firstArg == '_' else ''
-// FIXME(integers): pending optimizer work on handling the case where the
-// boolean value is wrapped into a two-case enum and then immediately


This seems to be a reference to the original ArithmeticOverflow enum that was mercifully removed. The block remains disabled because the holdup is with overload resolution, I think, and nothing to do with optimization of two-case enums.

xwu · 2018-01-31T03:08:04Z

@swift-ci Please smoke test

natecook1000

Thanks for fixing these, @xwu — I have one group of changes and a question for Max before we merge these.

natecook1000 · 2018-01-31T20:22:00Z

stdlib/public/core/Integers.swift.gyb

-  /// Returns the sum of this value and the given value along with a flag
-  /// indicating whether overflow occurred in the operation.
+  /// Returns the sum of this value and the given value, along with a Boolean
+  /// flag indicating whether overflow occurred in the operation.


Good catch on the "flag" business—these should all be "Boolean value indicating..." throughout to match the rest of the docs.

natecook1000 · 2018-01-31T20:23:04Z

stdlib/public/core/DoubleWidth.swift.gyb

  @_inlineable // FIXME(sil-serialize-all)
  @_transparent
-  public // @testable
-  init(_ _value: (High, Low)) {
+  public init(_ value: (high: High, low: Low)) {


I didn't even notice that this wasn't a public initializer when I wrote that—seems like a pretty intuitive usage of the type.

@moiseev, what do you think of making this public API?

xwu · 2018-02-01T19:58:38Z

@swift-ci Please smoke test

moiseev · 2018-02-01T19:57:25Z

stdlib/public/core/DoubleWidth.swift.gyb

-/// instances, in particular, can result in undesirable performance.
+/// The `DoubleWidth` type is not intended as a replacement for a variable-width
+/// integer type. Nesting `DoubleWidth` instances, in particular, may result in
+/// undesirable performance.


To me "undesirable performance" sounds like "you'll get too much performance even if you don't need it" ;-)

After I’m done with it, maybe you will... ;)

Update and fill in documentation for DoubleWidth and integer protocols

78875d9

xwu commented Jan 31, 2018

View reviewed changes

xwu requested a review from natecook1000 January 31, 2018 03:07

natecook1000 reviewed Jan 31, 2018

View reviewed changes

Address reviewer feedback

21d59c4

moiseev approved these changes Feb 1, 2018

View reviewed changes

xwu merged commit 37beb71 into swiftlang:master Feb 1, 2018

xwu deleted the integer-documentation branch February 1, 2018 22:07

natecook1000 mentioned this pull request Feb 21, 2018

[4.1] Update standard library docs #14768

Merged

[docs] Update and fill in documentation for DoubleWidth and integer protocols #14295

[docs] Update and fill in documentation for DoubleWidth and integer protocols #14295

Uh oh!

Conversation

xwu commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

moiseev Feb 1, 2018 • 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!

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!

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!

xwu commented Jan 31, 2018

Uh oh!

natecook1000 left a comment

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!

xwu commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xwu commented Jan 31, 2018 •

edited

Loading

moiseev Feb 1, 2018 •

edited

Loading