Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 1, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78875d9
Update and fill in documentation for DoubleWidth and integer protocols
xwu Jan 31, 2018
21d59c4
Address reviewer feedback
xwu Jan 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions stdlib/public/core/DoubleWidth.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,21 +10,22 @@
//
//===----------------------------------------------------------------------===//

/// A fixed-width integer that is twice the size of its base type.
/// A fixed-width integer that has twice the bit width of its base type.
///
/// 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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// result.
///
/// let a = 2241543570477705381
/// let b = 186319822866995413
/// let c = a.multipliedFullWidth(by: b)
/// // c == (22640526660490081, 7959093232766896457)
/// // c == (high: 22640526660490081, low: 7959093232766896457)
///
/// The tuple `c` can't be used in any further comparisons or calculations. To
/// use this value, create a `DoubleWidth` instance from the result. You can
/// use the `DoubleWidth` instance the way you use any other integer type.
/// use the `DoubleWidth` instance in the same way that you would use any other
/// integer type.
///
/// let d = DoubleWidth(a.multipliedFullWidth(by: b))
/// // d == 417644001000058515200174966092417353
Expand All @@ -40,9 +41,9 @@
/// }
/// // Prints "Too big to be an 'Int'!"
///
/// The `DoubleWidth` type is intended for intermediate calculations, not as a
/// replacement for a variable-width integer type. Nesting `DoubleWidth`
/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@_fixed_layout // FIXME(sil-serialize-all)
public struct DoubleWidth<Base : FixedWidthInteger> :
_ExpressibleByBuiltinIntegerLiteral
Expand All @@ -60,26 +61,31 @@ public struct DoubleWidth<Base : FixedWidthInteger> :
internal var _storage: (low: Low, high: High)
#endif

/// The high part of the value.
@_inlineable // FIXME(sil-serialize-all)
@_transparent
public var high: High {
return _storage.high
}

/// The low part of the value.
@_inlineable // FIXME(sil-serialize-all)
@_transparent
public var low: Low {
return _storage.low
}

/// Creates a new instance from the given tuple of high and low parts.
///
/// - Parameter value: The tuple to use as the source of the new instance's
/// high and low parts.
@_inlineable // FIXME(sil-serialize-all)
@_transparent
public // @testable
init(_ _value: (High, Low)) {
public init(_ value: (high: High, low: Low)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@moiseev moiseev Feb 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. I think this PR is ready then :)

#if _endian(big)
self._storage = (high: _value.0, low: _value.1)
self._storage = (high: value.0, low: value.1)
#else
self._storage = (low: _value.1, high: _value.0)
self._storage = (low: value.1, high: value.0)
#endif
}

Expand Down
103 changes: 49 additions & 54 deletions stdlib/public/core/Integers.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,8 +809,8 @@ def assignmentOperatorComment(operator, fixedWidth):
def overflowOperationComment(operator):
comments = {
'+': """\
/// 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
/// value indicating whether overflow occurred in the operation.
///
/// - Parameter rhs: The value to add to this value.
/// - Returns: A tuple containing the result of the addition along with a
Expand All @@ -821,32 +821,34 @@ def overflowOperationComment(operator):
/// and `rhs`.
""",
'-': """\
/// Returns the difference of this value and the given value along with a
/// flag indicating whether overflow occurred in the operation.
/// Returns the difference obtained by subtracting the given value from this
/// value, along with a Boolean value indicating whether overflow occurred in
/// the operation.
///
/// - Parameter rhs: The value to subtract from this value.
/// - Returns: A tuple containing the result of the subtraction along with a
/// flag indicating whether overflow occurred. If the `overflow` component
/// is `false`, the `partialValue` component contains the entire
/// difference. If the `overflow` component is `true`, an overflow
/// occurred and the `partialValue` component contains the truncated
/// result of `rhs` subtracted from this value.
/// Boolean value indicating whether overflow occurred. If the `overflow`
/// component is `false`, the `partialValue` component contains the entire
/// difference. If the `overflow` component is `true`, an overflow occurred
/// and the `partialValue` component contains the truncated result of `rhs`
/// subtracted from this value.
""",
'*': """\
/// Returns the product of this value and the given value along with a flag
/// indicating whether overflow occurred in the operation.
/// Returns the product of this value and the given value, along with a
/// Boolean value indicating whether overflow occurred in the operation.
///
/// - Parameter rhs: The value to multiply by this value.
/// - Returns: A tuple containing the result of the multiplication along with
/// a Boolean value indicating whether overflow occurred. If the `overflow`
/// component is `false`, the `partialValue` component contains the entire
/// product. If the `overflow` component is `true`, an overflow
/// occurred and the `partialValue` component contains the truncated
/// product of this value and `rhs`.
/// product. If the `overflow` component is `true`, an overflow occurred and
/// the `partialValue` component contains the truncated product of this
/// value and `rhs`.
""",
'/': """\
/// Returns the quotient of dividing this value by the given value along with
/// a flag indicating whether overflow occurred in the operation.
/// Returns the quotient obtained by dividing this value by the given value,
/// along with a Boolean value indicating whether overflow occurred in the
/// operation.
///
/// Dividing by zero is not an error when using this method. For a value `x`,
/// the result of `x.dividedReportingOverflow(by: 0)` is `(x, true)`.
Expand All @@ -856,22 +858,24 @@ def overflowOperationComment(operator):
/// Boolean value indicating whether overflow occurred. If the `overflow`
/// 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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This, I think, is a key clarification.

""",
'%': """\
// FIXME(integers): the comment is for division instead of remainder
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// Returns the remainder of dividing this value by the given value along
/// with a flag indicating whether overflow occurred in the operation.
/// Returns the remainder after dividing this value by the given value, along
/// with a Boolean value indicating whether overflow occurred during division.
///
/// Dividing by zero is not an error when using this method. For a value `x`,
/// the result of `x.dividedReportingOverflow(by: 0)` is `(x, true)`.
/// the result of `x.remainderReportingOverflow(dividingBy: 0)` is
/// `(x, true)`.
///
/// - Parameter rhs: The value to divide this value by.
/// - Returns: A tuple containing the result of the division along with a
/// - Returns: A tuple containing the result of the operation along with a
/// Boolean value indicating whether overflow occurred. If the `overflow`
/// 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.
/// remainder. If the `overflow` component is `true`, an overflow occurred
/// during division and the `partialValue` component contains either the
/// entire remainder or, if the remainder is undefined, the dividend.
""",
}
return comments[operator]
Expand All @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// value without checking for arithmetic overflow.
///
/// If an arithmetic overflow occurs, the behavior is undefined. Use this
/// function only to avoid the cost of overflow checking when you are sure
/// that the operation won't overflow.
///
/// - Parameter rhs: The value to subtract from this value.
/// - Returns: The difference of this value and `rhs`.
/// - Returns: The result of subtracting `rhs` from this value.
""",
'*': """\
/// Returns the product of this value and the given value without checking
Expand All @@ -915,18 +919,18 @@ def unsafeOperationComment(operator):
/// that the operation won't overflow.
///
/// - Parameter rhs: The value to multiply by this value.
/// - Returns: The difference of this value and `rhs`.
/// - Returns: The product of this value and `rhs`.
""",
'/': """\
/// Returns the quotient of dividing this value by the given value without
/// checking for arithmetic overflow.
/// Returns the quotient obtained by dividing this value by the given value
/// without checking for arithmetic overflow.
///
/// If an arithmetic overflow occurs, the behavior is undefined. Use this
/// function only to avoid the cost of overflow checking when you are sure
/// that the operation won't overflow.
///
/// - Parameter rhs: The value to divide this value by.
/// - Returns: The quotient of dividing this value by `rhs`.
/// - Returns: The result of dividing this value by `rhs`.
""",
}
return comments[operator]
Expand Down Expand Up @@ -2084,7 +2088,7 @@ public protocol FixedWidthInteger :
/// `-(2 ** (bitWidth - 1))` through `(2 ** (bitWidth - 1)) - 1`. For example,
/// the `Int8` type has a `bitWidth` value of 8 and can store any integer in
/// the range `-128...127`.
static var bitWidth : Int { get }
static var bitWidth: Int { get }

/// The maximum representable integer in this type.
///
Expand Down Expand Up @@ -2112,10 +2116,10 @@ ${overflowOperationComment(x.operator)}
///
/// Use this method to calculate the full result of a product that would
/// otherwise overflow. Unlike traditional truncating multiplication, the
/// `multipliedFullWidth(by:)` method returns a tuple
/// containing both the `high` and `low` parts of the product of this value and
/// `other`. The following example uses this method to multiply two `UInt8`
/// values that normally overflow when multiplied:
/// `multipliedFullWidth(by:)` method returns a tuple containing both the
/// `high` and `low` parts of the product of this value and `other`. The
/// following example uses this method to multiply two `UInt8` values that
/// normally overflow when multiplied:
///
/// let x: UInt8 = 100
/// let y: UInt8 = 20
Expand All @@ -2136,20 +2140,18 @@ ${overflowOperationComment(x.operator)}
/// - Returns: A tuple containing the high and low parts of the result of
/// multiplying this value and `other`.
func multipliedFullWidth(by other: Self) -> (high: Self, low: Self.Magnitude)
// FIXME(integers): figure out how to return DoubleWidth<Self>

/// Returns a tuple containing the quotient and remainder of dividing the
/// given value by this value.
/// Returns a tuple containing the quotient and remainder obtained by dividing
/// the given value by this value.
///
/// The resulting quotient must be representable within the bounds of the
/// type. If the quotient of dividing `dividend` by this value is too large
/// to represent in the type, a runtime error may occur.
/// type. If the quotient is too large to represent in the type, a runtime
/// error may occur.
///
/// - 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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// - Returns: A tuple containing the quotient and remainder of `dividend`
/// divided by this value.
/// double-width integer.
/// - Returns: A tuple containing the quotient and remainder obtained by
/// dividing `dividend` by this value.
func dividingFullWidth(_ dividend: (high: Self, low: Self.Magnitude))
-> (quotient: Self, remainder: Self)

Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// unwrapped. <rdar://problem/29004429>
// Uncomment this block and remove the corresponding one from the concrete
// types once the optimizer is ready.
// FIXME(integers): uncomment this block and remove the corresponding one from
// the concrete types
#if false
${assignmentOperatorComment(x.operator, True)}
@_transparent
Expand Down Expand Up @@ -3085,9 +3084,6 @@ public struct ${Self}
return Bool(Builtin.cmp_${u}lt_Int${bits}(lhs._value, rhs._value))
}

// FIXME(integers): pending optimizer work on handling the case where the
// boolean value is wrapped into a two-case enum and then immediately
// unwrapped. <rdar://problem/29004429>
// See corresponding definitions in the FixedWidthInteger extension.
% for x in binaryArithmetic['Numeric'] + binaryArithmetic["BinaryInteger"][:1]:
${assignmentOperatorComment(x.operator, True)}
Expand Down Expand Up @@ -3121,7 +3117,6 @@ ${assignmentOperatorComment(x.operator, True)}
lhs = ${Self}(result)
}
% end
// end of FIXME(integers)

% for x in chain(*binaryArithmetic.values()):

Expand Down