Add RangeSet and discontiguous collection operations #28161

natecook1000 · 2019-11-08T22:14:49Z

This adds the RangeSet and DiscontiguousSlice types, as well as collection operations for working with discontiguous ranges of elements. This also adds a COWLoggingArray type to the test suite to verify that mutable collection algorithms don't perform unexpected copy-on-write operations when mutating slices mid-operation.

Still to do:

Add documentation for RangeSet set algebra methods (since we aren't conforming, we don't inherit the docs from the protocol)
Add versioning attributes

This adds the RangeSet and DiscontiguousSlice types, as well as collection operations for working with discontiguous ranges of elements. This also adds a COWLoggingArray type to the test suite to verify that mutable collection algorithms don't perform unexpected copy-on-write operations when mutating slices mid-operation.

natecook1000 · 2019-11-13T19:38:07Z

@swift-ci Please smoke test

natecook1000 · 2020-02-10T21:15:21Z

@swift-ci Please smoke test

natecook1000 · 2020-02-10T22:33:42Z

@swift-ci Please smoke test

Just use a `Range<T>` as is for the single-range case. If there's a benefit to making range sets more compact we can move toward that instead.

natecook1000 · 2020-02-14T23:18:23Z

@swift-ci Please smoke test

xwu · 2020-02-15T01:56:39Z

stdlib/public/core/RangeSet.swift

-/// A set of ranges of any comparable value.
+/// A set of any comparable value, represented by ranges.
+///
+/// You can use a range set to efficiently represent a set `Comparable` values


“set of”

xwu · 2020-02-15T01:57:47Z

stdlib/public/core/RangeSet.swift

+//
+//===----------------------------------------------------------------------===//
+
+/// A set of any comparable value, represented by ranges.


“any comparable value__s__”?

natecook1000 · 2020-02-17T13:44:51Z

@swift-ci Please smoke test

This method was removed from the final approved SE proposal.

milseman

Talked with Nate on the phone, recording some points here.

milseman · 2020-02-21T17:56:11Z

stdlib/public/core/RangeSet.swift

+  /// - `_indicesOfRange(12..<19) == 1..<2`
+  /// - `_indicesOfRange(17..<19) == 2..<2`
+  /// - `_indicesOfRange(12..<22) == 1..<3`
+  func _indicesOfRange(_ range: Range<Bound>) -> Range<Int> {


Explicit internal

milseman · 2020-02-21T17:56:36Z

stdlib/public/core/CollectionAlgorithms.swift

+  /// - Complexity: O(*n* log *n*) where *n* is the number of elements.
+  /// - Precondition:
+  ///   `n == distance(from: range.lowerBound, to: range.upperBound)`
+  @usableFromInline


Can probably drop

milseman · 2020-02-21T17:56:45Z

stdlib/public/core/CollectionAlgorithms.swift

+  ///
+  /// - Complexity: O(log *n*), where *n* is the length of this collection if
+  ///   the collection conforms to `RandomAccessCollection`, otherwise O(*n*).
+  @usableFromInline


Can probably drop

milseman · 2020-02-21T17:57:48Z

stdlib/public/core/MutableCollection.swift

+  /// - Complexity: O(*n* log *n*) where *n* is the length of the collection.
+  @available(macOS 9999, iOS 9999, tvOS 9999, watchOS 9999, *)
+  @discardableResult
+  @inlinable


Drop inlinable, doesn't buy us anything as written. If you add the rotate fast path, then hand-outline the rest of this code so it can be inlined. The end result is the same at run-time for users, but we have less code size and branches in user code.

milseman · 2020-02-21T17:57:59Z

stdlib/public/core/MutableCollection.swift

+  /// - Returns: The new index of the element that was first pre-rotation.
+  ///
+  /// - Complexity: O(*n*)
+  @usableFromInline


Can probably drop

milseman · 2020-02-21T18:04:02Z

stdlib/public/core/RangeSet.swift

+  ///   - index: The index to include in the range set. `index` must be a
+  ///     valid index of `collection` that isn't the collection's `endIndex`.
+  ///   - collection: The collection that contains `index`.
+  public init<S, C>(_ indices: S, within collection: C)


This could be a candidate for inlinable, depending on need. It's a bigger code size tradeoff than insert below.

milseman · 2020-02-21T18:04:24Z

stdlib/public/core/RangeSet.swift

+  ///
+  /// - Complexity: O(*n*), where *n* is the number of ranges in the range
+  ///   set.
+  public mutating func remove<C>(_ index: Bound, within collection: C)


inlinable could be a win.

milseman · 2020-02-21T18:04:46Z

stdlib/public/core/RangeSet.swift

+  /// - Complexity: O(*n*), where *n* is the number of ranges in the range
+  ///   set.
+  @usableFromInline
+  internal func _inverted<C>(within collection: C) -> RangeSet


Candidate for inlinable if this is called heavily in other inlinable code.

milseman · 2020-02-21T18:06:52Z

stdlib/public/core/RangeSet.swift

+internal struct _Pair<Element>: RandomAccessCollection {
+  var pair: (first: Element, second: Element)
+
+  init(_ first: Element, _ second: Element) {


explicit internal on all these entities

milseman · 2020-02-21T18:07:04Z

stdlib/public/core/RangeSetStorage.swift

+//
+//===----------------------------------------------------------------------===//
+
+struct _RangeSetStorage<T: Comparable> {


explicit internal

In particular, only adding inlining when functions can reduce or eliminate generic parameters. I'll follow up with benchmarks to test the effects of adding more inlinability.

natecook1000 · 2020-02-21T19:04:28Z

@swift-ci Please smoke test

kylemacomber · 2020-02-21T20:42:25Z

stdlib/public/core/DiscontiguousSlice.swift

+  where Base: BidirectionalCollection
+{
+  public func index(before i: Index) -> Index {
+    _precondition(i != startIndex, "Can't move index before startIndex")


Do we have a policy for when we include out of bounds checks in index(after:)/index(before:)?

kylemacomber · 2020-02-21T20:46:51Z

stdlib/public/core/MutableCollection.swift

+  ///   - distance(from: lhs.lowerBound, to: p) == distance(from:
+  ///     rhs.lowerBound, to: q)
+  ///   - p == lhs.upperBound || q == rhs.upperBound
+  mutating func _swapNonemptySubrangePrefixes(


Nit: missing an explicit internal

With availability annotations, the RangeSet subscript overloads aren't coming through for these tests.

natecook1000 · 2020-02-22T06:45:26Z

@swift-ci Please smoke test

natecook1000 · 2020-02-22T16:44:17Z

@swift-ci Please smoke test

drodriguez · 2020-02-23T00:34:53Z

@compnerd: I am far from a real computer, but it seems that the Windows failures might simply be fixed moving the RangeSet.swift from test to validation-test. I only see import StdlibCollectionUnitests in the validation-test directory. I cannot find where that path is set, though.

compnerd · 2020-02-23T00:47:08Z

Validation test just effectively disables it (I need to rebase my patch to enable that on the Azure agents). Could just be something we are setting for that test suite. I'm waiting for an update to complete; Ill look at it once the update is complete.

compnerd · 2020-02-23T01:23:37Z

I think that there is a missing dependency

…8161)" This reverts commit c6183ee.

* Revert "test: move RangeSet test into validation-test" This reverts commit e9aaef4. * Revert "Add RangeSet and discontiguous collection operations (#28161)" This reverts commit c6183ee.

lugeke · 2020-08-04T01:41:50Z

stdlib/public/core/RangeSetStorage.swift

+    switch (lhs._storage, rhs._storage) {
+    case (.empty, .empty):
+      return true
+    case (.empty, .singleRange), (.singleRange, .empty):


When case .singleRange is empty, doesn’t it equal to case .empty?

natecook1000 added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Nov 8, 2019

Fix broken invariant in single-range initializer

1cbe083

natecook1000 added 3 commits January 16, 2020 10:55

Merge branch 'master' into nc_rangeset

05be13e

Revise proposed RangeSet & collections API

f45e0ab

Revise proposed RangeSet & collections API for proposal v3

1ac6f07

natecook1000 mentioned this pull request Jan 22, 2020

Update SE-0270 proposal with 3rd revision apple/swift-evolution#1115

Merged

theblixguy added swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process and removed swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review labels Feb 5, 2020

natecook1000 added 3 commits February 10, 2020 12:43

Merge branch 'master' into nc_rangeset

63f878d

[stdlib] Adopt the custom RangeSet storage type

574c94c

Update copyright notices

b60223b

Switch to standard library preconditions

5321c34

natecook1000 force-pushed the nc_rangeset branch from 9481251 to 5321c34 Compare February 10, 2020 22:33

natecook1000 added 5 commits February 11, 2020 00:16

Merge branch 'master' into nc_rangeset

4b43d8c

Remove ExpressibleByArrayLiteral conformance

6c742f5

Add availability attributes to RangeSet &co

d06d089

Simplify RangeSetStorage's single-range case

08f5773

Just use a `Range<T>` as is for the single-range case. If there's a benefit to making range sets more compact we can move toward that instead.

Fill in remaining RangeSet documentation

c386444

natecook1000 marked this pull request as ready for review February 14, 2020 23:17

natecook1000 requested review from lorentey and kylemacomber February 14, 2020 23:17

xwu reviewed Feb 15, 2020

View reviewed changes

natecook1000 added 2 commits February 15, 2020 09:12

Minor revisions to RangeSet type docs

62c3348

Update test fixtures for RangeSet subscript

e09b1a8

natecook1000 added 5 commits February 17, 2020 10:58

Fix a documentation type

7c46522

Remove RangeSet.contains(_: Range) method

0723c4f

This method was removed from the final approved SE proposal.

Switch remaining preconditions to internal stdlib versions

a28cff9

Inversion of empty rangeset should be the whole bounds

bb0f357

Inlinability annotations

348eeff

milseman reviewed Feb 21, 2020

View reviewed changes

Revise inlining annotations after discussion w/ @milseman

41ca5b4

In particular, only adding inlining when functions can reduce or eliminate generic parameters. I'll follow up with benchmarks to test the effects of adding more inlinability.

kylemacomber reviewed Feb 21, 2020

View reviewed changes

natecook1000 added 3 commits February 21, 2020 21:28

Add missing internal annotation

64a32d9

Merge branch 'master' into nc_rangeset

d993755

Update test expectations

6eb76ed

With availability annotations, the RangeSet subscript overloads aren't coming through for these tests.

Add availability to test fixtures

8a5b441

kylemacomber approved these changes Feb 22, 2020

View reviewed changes

natecook1000 merged commit c6183ee into apple:master Feb 22, 2020

natecook1000 deleted the nc_rangeset branch February 22, 2020 21:33

natecook1000 added a commit to natecook1000/swift that referenced this pull request May 15, 2020

Revert "Add RangeSet and discontiguous collection operations (apple#2…

bcd4e49

…8161)" This reverts commit c6183ee.

lugeke reviewed Aug 4, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RangeSet and discontiguous collection operations #28161

Add RangeSet and discontiguous collection operations #28161

natecook1000 commented Nov 8, 2019 •

edited

natecook1000 commented Nov 13, 2019

natecook1000 commented Feb 10, 2020

natecook1000 commented Feb 10, 2020

natecook1000 commented Feb 14, 2020

xwu Feb 15, 2020

xwu Feb 15, 2020

natecook1000 commented Feb 17, 2020

milseman left a comment

milseman Feb 21, 2020

milseman Feb 21, 2020

milseman Feb 21, 2020

milseman Feb 21, 2020

milseman Feb 21, 2020

milseman Feb 21, 2020

milseman Feb 21, 2020

milseman Feb 21, 2020

milseman Feb 21, 2020

milseman Feb 21, 2020

natecook1000 commented Feb 21, 2020

kylemacomber Feb 21, 2020

kylemacomber Feb 21, 2020

natecook1000 commented Feb 22, 2020

natecook1000 commented Feb 22, 2020

drodriguez commented Feb 23, 2020

compnerd commented Feb 23, 2020

compnerd commented Feb 23, 2020

lugeke Aug 4, 2020

Add RangeSet and discontiguous collection operations #28161

Add RangeSet and discontiguous collection operations #28161

Conversation

natecook1000 commented Nov 8, 2019 • edited

natecook1000 commented Nov 13, 2019

natecook1000 commented Feb 10, 2020

natecook1000 commented Feb 10, 2020

natecook1000 commented Feb 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natecook1000 commented Feb 17, 2020

milseman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natecook1000 commented Feb 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natecook1000 commented Feb 22, 2020

natecook1000 commented Feb 22, 2020

drodriguez commented Feb 23, 2020

compnerd commented Feb 23, 2020

compnerd commented Feb 23, 2020

Choose a reason for hiding this comment

natecook1000 commented Nov 8, 2019 •

edited