[stdlib] Update complexity docs for seq/collection algorithms #17254

natecook1000 · 2018-06-15T20:51:57Z

This corrects and standardizes the complexity documentation for Sequence and Collection methods. The use of constants is more consistent, with n equal to the length of the target collection, m equal to the length of a collection passed in as a parameter, and k equal to any other passed
or calculated constant.

beccadax

I like the way you're standardizing the language, but I found a few places where I think you used the wrong name.

beccadax · 2018-06-15T22:52:43Z

stdlib/public/core/BidirectionalCollection.swift

-  ///   `RandomAccessCollection`; otherwise, O(*n*), where *n* is the length
-  ///   of the collection.
+  ///   `RandomAccessCollection`; otherwise, O(*n*), where *n* is the number of
+  ///   elements to remove.


Shouldn't the n be k under the rule you stated?

Thanks for the review, @brentdax! Fixed these.

beccadax · 2018-06-15T22:53:03Z

stdlib/public/core/BidirectionalCollection.swift

-  /// - Complexity: O(*n*), where *n* is the number of elements to drop.
+  /// - Complexity: O(1) if the collection conforms to
+  ///   `RandomAccessCollection`; otherwise, O(*n*), where *n* is the number of
+  ///   elements to drop.


beccadax · 2018-06-15T22:53:15Z

stdlib/public/core/BidirectionalCollection.swift

-  /// - Complexity: O(*n*), where *n* is equal to `maxLength`.
+  /// - Complexity: O(1) if the collection conforms to
+  ///   `RandomAccessCollection`; otherwise, O(*n*), where *n* is equal to
+  ///   `maxLength`.


beccadax · 2018-06-15T22:53:50Z

stdlib/public/core/Collection.swift

-  ///   the beginning of the collection.
+  /// - Complexity: O(1) if the collection conforms to
+  ///   `RandomAccessCollection`; otherwise, O(*n*), where *n* is the number of
+  ///   elements to drop from the beginning of the collection.


beccadax · 2018-06-15T22:53:58Z

stdlib/public/core/Collection.swift

-  /// - Complexity: O(*n*), where *n* is the length of the collection.
+  /// - Complexity: O(1) if the collection conforms to
+  ///   `RandomAccessCollection`; otherwise, O(*n*), where *n* is the number of
+  ///   elements to drop from the beginning of the collection.


beccadax · 2018-06-15T22:54:07Z

stdlib/public/core/Collection.swift

+  ///
+  /// - Complexity: O(1) if the collection conforms to
+  ///   `RandomAccessCollection`; otherwise, O(*n*), where *n* is the number of
+  ///   elements to select from the beginning of the collection.


dabrahams

Unlikely I'll have time to finish before I get back from vacation in a week

dabrahams · 2018-06-15T21:41:27Z

stdlib/public/core/Array.swift

-  /// - Complexity: Amortized O(1) over many additions. If the array uses a
-  ///   bridged `NSArray` instance as its storage, the efficiency is
-  ///   unspecified.
+  /// - Complexity: O(1) on average, over many additions to the same array.


"addition" seems like the wrong word. Why not just "append?"

"appends" is odd as a noun… To use append, we'd need to write something like "append operations" or "calls to append(_:)", neither of which seem better. What strikes you as wrong about "additions"?

dabrahams · 2018-06-15T21:42:31Z

stdlib/public/core/Array.swift

-  ///   unspecified.
+  /// - Complexity: O(1) on average, over many additions to the same array.
+  ///   If the array uses a bridged `NSArray` instance as its storage, the
+  ///   complexity is unspecified.


I wonder if this isn't also amortized O(1). Isn't it just as though the array started with no spare capacity?

That's true — you'd get one copy to native storage, and then you're amortized O(1) from there on out.

dabrahams · 2018-06-15T21:44:09Z

stdlib/public/core/Array.swift

@@ -1163,7 +1163,7 @@ extension Array: RangeReplaceableCollection, ArrayProtocol {
  ///
  /// - Parameter newElements: The elements to append to the array.
  ///
-  /// - Complexity: O(*n*), where *n* is the length of the resulting array.
+  /// - Complexity: O(*m*), where *m* is the length of `newElements`.


Did we not decide that this was amortized O(m)? I don't remember…

dabrahams · 2018-06-15T21:45:40Z

stdlib/public/core/Array.swift

-  /// - Complexity: O(*n*), where *n* is the length of the array.
+  /// - Complexity: O(*n*), where *n* is the length of the array. If the
+  ///   call to this method appends `newElement` to the array, the
+  ///   complexity is O(1) on average, over many additions to the same


why not say, "equivalent to append(newElement) if i == count"?

Otherwise s/addition/append/

dabrahams · 2018-06-15T21:46:46Z

stdlib/public/core/Array.swift

+  /// - Complexity: O(*n* + *m*), where *n* is length of the array and
+  ///   *m* is the length of `newElements`. If the call to this method simply
+  ///   appends the contents of `newElements` to the array, the complexity
+  ///   is O(*m*).


Another opportunity to use equivalence.

dabrahams · 2018-06-16T14:28:32Z

"Calls to append(_:)" seems better to me. "Additions" sounds like applications of the + operator when I read it.

…

Sent from my iPad

On Jun 15, 2018, at 5:56 PM, Nate Cook ***@***.***> wrote: @natecook1000 commented on this pull request. In stdlib/public/core/Array.swift: > @@ -1138,9 +1138,9 @@ extension Array: RangeReplaceableCollection, ArrayProtocol { /// /// - Parameter newElement: The element to append to the array. /// - /// - Complexity: Amortized O(1) over many additions. If the array uses a - /// bridged `NSArray` instance as its storage, the efficiency is - /// unspecified. + /// - Complexity: O(1) on average, over many additions to the same array. "appends" is odd as a noun… To use append, we'd need to write something like "append operations" or "calls to append(_:)", neither of which seem better. What strikes you as wrong about "additions"? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

dabrahams · 2018-07-12T10:49:14Z

stdlib/public/core/BidirectionalCollection.swift

-  ///   `RandomAccessCollection`; otherwise, O(*n*), where *n* is the length
-  ///   of the collection.
+  ///   `RandomAccessCollection`; otherwise, O(*k*), where *k* is the number of
+  ///   elements to remove.
  @inlinable // protocol-only
  public mutating func removeLast(_ n: Int) {


I don't think we should change the doc in this way without also changing the parameter name to k.

dabrahams · 2018-07-12T10:50:01Z

stdlib/public/core/BidirectionalCollection.swift

-  /// - Complexity: O(*n*), where *n* is the number of elements to drop.
+  /// - Complexity: O(1) if the collection conforms to
+  ///   `RandomAccessCollection`; otherwise, O(*k*), where *k* is the number of
+  ///   elements to drop.
  @inlinable // protocol-only
  public func dropLast(_ n: Int) -> SubSequence {


I don't think we should change the doc in this way without also changing the parameter name to k.

dabrahams · 2018-07-12T10:52:36Z

stdlib/public/core/Array.swift

-  /// The value passed as `n` must not offset `i` beyond the bounds of the
-  /// collection.
+  /// The value passed as `distance` must not offset `i` beyond the bounds of
+  /// the collection.


a. Shouldn't this be listed as a precondition?
b. The phrasing is a bit weird, since the value passed doesn't itself offset anything. Can we do better?

Agreed! I'm going to take this up on a different branch, since that's existing language and this branch keeps falling behind master.

@natecook1000 Looks like this was never resolved; master still contains this language.

natecook1000 · 2018-07-17T18:47:03Z

@swift-ci Please smoke test

natecook1000 · 2018-07-18T18:12:50Z

@swift-ci Please smoke test

This corrects and standardizes the complexity documentation for Sequence and Collection methods. The use of constants is more consistent, with `n` equal to the length of the target collection, `m` equal to the length of a collection passed in as a parameter, and `k` equal to any other passed or calculated constant.

natecook1000 · 2018-07-23T21:07:49Z

@swift-ci Please smoke test

@brentdax

…ang#17254) * [stdlib] Update complexity docs for seq/collection algorithms This corrects and standardizes the complexity documentation for Sequence and Collection methods. The use of constants is more consistent, with `n` equal to the length of the target collection, `m` equal to the length of a collection passed in as a parameter, and `k` equal to any other passed or calculated constant. * Apply notes from @brentdax about complexity nomenclature * Change `n` to `distance` in `index(_:offsetBy:)` * Use equivalency language more places; sync across array types * Use k instead of n for parameter names * Slight changes to index(_:offsetBy:) discussion. * Update tests with new parameter names

@brentdax

#18247) * [stdlib] Update complexity docs for seq/collection algorithms This corrects and standardizes the complexity documentation for Sequence and Collection methods. The use of constants is more consistent, with `n` equal to the length of the target collection, `m` equal to the length of a collection passed in as a parameter, and `k` equal to any other passed or calculated constant. * Apply notes from @brentdax about complexity nomenclature * Change `n` to `distance` in `index(_:offsetBy:)` * Use equivalency language more places; sync across array types * Use k instead of n for parameter names * Slight changes to index(_:offsetBy:) discussion. * Update tests with new parameter names

natecook1000 requested a review from dabrahams June 15, 2018 20:52

beccadax requested changes Jun 15, 2018

View reviewed changes

dabrahams reviewed Jun 15, 2018

View reviewed changes

natecook1000 changed the title ~~[stdlib] Update complexity docs for seq/collection algorithms~~ [WIP] [stdlib] Update complexity docs for seq/collection algorithms Jun 16, 2018

natecook1000 force-pushed the nc-complexity branch 4 times, most recently from 74fed4d to f90f75a Compare July 11, 2018 15:21

beccadax approved these changes Jul 12, 2018

View reviewed changes

dabrahams reviewed Jul 12, 2018

View reviewed changes

natecook1000 force-pushed the nc-complexity branch from 70631f0 to 5f53908 Compare July 17, 2018 16:09

natecook1000 changed the title ~~[WIP] [stdlib] Update complexity docs for seq/collection algorithms~~ [stdlib] Update complexity docs for seq/collection algorithms Jul 17, 2018

natecook1000 force-pushed the nc-complexity branch from 5f53908 to e651dda Compare July 18, 2018 18:12

natecook1000 added 7 commits July 23, 2018 16:05

Apply notes from @brentdax about complexity nomenclature

522f385

Change n to distance in index(_:offsetBy:)

2a75183

Use equivalency language more places; sync across array types

983b24a

Use k instead of n for parameter names

a0917b8

Slight changes to index(_:offsetBy:) discussion.

29366d0

Update tests with new parameter names

bcab652

natecook1000 force-pushed the nc-complexity branch from 004f96a to bcab652 Compare July 23, 2018 21:07

natecook1000 merged commit 3d7dfc2 into swiftlang:master Jul 24, 2018

natecook1000 mentioned this pull request Jul 26, 2018

[4.2] [stdlib] Update complexity docs for seq/collection algorithms #18247

Merged

natecook1000 deleted the nc-complexity branch October 4, 2018 15:23

[stdlib] Update complexity docs for seq/collection algorithms #17254

[stdlib] Update complexity docs for seq/collection algorithms #17254

Uh oh!

Conversation

natecook1000 commented Jun 15, 2018

Uh oh!

beccadax 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!

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!

dabrahams 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!

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!

dabrahams commented Jun 16, 2018 via email

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!

natecook1000 commented Jul 17, 2018

Uh oh!

natecook1000 commented Jul 18, 2018

Uh oh!

natecook1000 commented Jul 23, 2018

Uh oh!

Uh oh!