Skip to content

stdlib: Remove Collection.randomElement customization point. #17637

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 1 commit into from
Jun 30, 2018
Merged

stdlib: Remove Collection.randomElement customization point. #17637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 0 additions & 19 deletions stdlib/public/core/Collection.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,25 +796,6 @@ public protocol Collection: Sequence where SubSequence: Collection {
/// - Parameter i: A valid index of the collection. `i` must be less than
/// `endIndex`.
func formIndex(after i: inout Index)

/// Returns a random element of the collection, using the given generator as
/// a source for randomness.
///
/// You use this method to select a random element from a collection when you
/// are using a custom random number generator. For example, call
/// `randomElement(using:)` to select a random element from an array of names.
///
/// let names = ["Zoey", "Chloe", "Amani", "Amaia"]
/// let randomName = names.randomElement(using: &myGenerator)!
/// // randomName == "Amani"
///
/// - Parameter generator: The random number generator to use when choosing
/// a random element.
/// - Returns: A random element from the collection. If the collection is
/// empty, the method returns `nil`.
__consuming func randomElement<T: RandomNumberGenerator>(
using generator: inout T
) -> Element?
}

/// Default implementation for forward collections.
Expand Down
118 changes: 30 additions & 88 deletions stdlib/public/core/Integers.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2535,93 +2535,6 @@ ${assignmentOperatorComment(x.operator, False)}
% for Range in ['Range', 'ClosedRange']:
% exampleRange = '1..<100' if Range == 'Range' else '1...100'

extension ${Range}
where Bound: FixedWidthInteger, Bound.Stride : SignedInteger,
Bound.Magnitude: UnsignedInteger
{

/// Returns a random element of the range, using the given generator as
/// a source for randomness.
///
/// You can use this method to select a random element of a range when you
/// are using a custom random number generator. If you're generating a random
/// number, in most cases, you should prefer using the `random(in:using:)`
/// static method of the desired numeric type. That static method is available
/// for both integer and floating point types, and returns a non-optional
/// value.
///
/// - Parameter generator: The random number generator to use when choosing
/// a random element.
/// - Returns: A random element of the range.
% if 'Closed' not in Range:
/// If the range is empty, the method returns `nil`.
% else:
/// This method never returns `nil`.
% end
@inlinable
public func randomElement<T: RandomNumberGenerator>(
using generator: inout T
) -> Element? {
guard !isEmpty else {
return nil
}
// Compute delta, the distance between the lower and upper bounds. This
// value may not representable by the type Bound if Bound is signed, but
// is always representable as Bound.Magnitude.
% if 'Closed' in Range:
var delta = Bound.Magnitude(truncatingIfNeeded: upperBound &- lowerBound)
% else:
let delta = Bound.Magnitude(truncatingIfNeeded: upperBound &- lowerBound)
% end
% if 'Closed' in Range:
// Subtle edge case: if the range is the whole set of representable values,
// then adding one to delta to account for a closed range will overflow.
// If we used &+ instead, the result would be zero, which isn't helpful,
// so we actually need to handle this case separately.
if delta == Bound.Magnitude.max {
return Bound(truncatingIfNeeded: generator.next() as Bound.Magnitude)
}
// Need to widen delta to account for the right-endpoint of a closed range.
delta += 1
% end
// The mathematical result we want is lowerBound plus a random value in
// 0 ..< delta. We need to be slightly careful about how we do this
// arithmetic; the Bound type cannot generally represent the random value,
// so we use a wrapping addition on Bound.Magnitude. This will often
// overflow, but produces the correct bit pattern for the result when
// converted back to Bound.
return Bound(truncatingIfNeeded:
Bound.Magnitude(truncatingIfNeeded: lowerBound) &+
generator.next(upperBound: delta)
)
}

/// Returns a random element of the range, using the given generator as
/// a source for randomness.
///
/// You can use this method to select a random element of a range when you
/// are using a custom random number generator. If you're generating a random
/// number, in most cases, you should prefer using the `random(in:)`
/// static method of the desired numeric type. That static method is available
/// for both integer and floating point types, and returns a non-optional
/// value.
///
/// This method uses the default random generator, `Random.default`. Calling
/// `(${exampleRange}).randomElement()` is equivalent to calling
/// `(${exampleRange}).randomElement(using: &Random.default)`.
///
/// - Returns: A random element of the range.
% if 'Closed' not in Range:
/// If the range is empty, the method returns `nil`.
% else:
/// This method never returns `nil`.
% end
@inlinable
public func randomElement() -> Element? {
return randomElement(using: &Random.default)
}
}

extension FixedWidthInteger
where Self.Stride : SignedInteger,
Self.Magnitude : UnsignedInteger {
Expand Down Expand Up @@ -2657,7 +2570,36 @@ where Self.Stride : SignedInteger,
!range.isEmpty,
"Can't get random value with an empty range"
)
return range.randomElement(using: &generator)!

// Compute delta, the distance between the lower and upper bounds. This
// value may not representable by the type Bound if Bound is signed, but
// is always representable as Bound.Magnitude.
% if 'Closed' in Range:
var delta = Magnitude(truncatingIfNeeded: range.upperBound &- range.lowerBound)
% else:
let delta = Magnitude(truncatingIfNeeded: range.upperBound &- range.lowerBound)
% end
% if 'Closed' in Range:
// Subtle edge case: if the range is the whole set of representable values,
// then adding one to delta to account for a closed range will overflow.
// If we used &+ instead, the result would be zero, which isn't helpful,
// so we actually need to handle this case separately.
if delta == Magnitude.max {
return Self(truncatingIfNeeded: generator.next() as Magnitude)
}
// Need to widen delta to account for the right-endpoint of a closed range.
delta += 1
Copy link
Contributor

Choose a reason for hiding this comment

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

@stephentyrone It looks like the optimizer does figure out that it is not necessary to check for the overflow, because you did it manually in the if block above… but still, wouldn't it make sense to go with delta &+= 1 here?

% end
// The mathematical result we want is lowerBound plus a random value in
// 0 ..< delta. We need to be slightly careful about how we do this
// arithmetic; the Bound type cannot generally represent the random value,
// so we use a wrapping addition on Bound.Magnitude. This will often
// overflow, but produces the correct bit pattern for the result when
// converted back to Bound.
return Self(truncatingIfNeeded:
Magnitude(truncatingIfNeeded: range.lowerBound) &+
generator.next(upperBound: delta)
)
}

/// Returns a random value within the specified range.
Expand Down