[stdlib] Implementing O(1) methods on `CountableClosedRange` #2993

austinzheng · 2016-06-12T05:42:56Z

What's in this pull request?

Functional changes and additional unit test coverage for CountableRange and CountableClosedRange.

All tests pass on OS X.

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

Changes:

Implemented O(1) index(_:offsetBy:) and distance(from:to:) methods on CountableClosedRange
Fixed some unit tests
Added many more unit tests for Countable*Range index APIs

moiseev · 2016-06-13T20:10:52Z

@swift-ci Please test

moiseev · 2016-06-13T20:17:39Z

stdlib/public/core/ClosedRange.swift

+      return 1 + limit.distance(to: right)
+    case (.pastEnd, .pastEnd):
+      // end <--> end
+      return limit.distance(to: limit)


Bound.Stride conforms to Integer, so this can be 0 I guess?

moiseev · 2016-06-13T20:32:21Z

Thanks @austinzheng for doing this. Please let me know once you've updated the tests and I'll be happy to merge it in.

austinzheng · 2016-06-13T22:04:12Z

I also have another PR, whenever you or Dmitri have time: #2940

austinzheng · 2016-06-14T02:34:05Z

I need to rewrite a bunch of these tests. I'll ping you when I have the updated code ready.

austinzheng · 2016-06-14T04:56:26Z

@moiseev

I fixed the issues raised in your comments, addressed some additional bugs, and rewrote some of the test suites I added since they were not properly implemented before. The tests in question pass; I'm running the entire test suite on OS X right now and will comment again when it finishes. Otherwise, it's ready.

austinzheng · 2016-06-14T05:38:37Z

All tests pass on my OS X machine.

moiseev · 2016-06-14T17:00:46Z

validation-test/stdlib/Range.swift.gyb

+      }
+      previousValue = range[idx].value
+    }
+    // Iterate once more to get to the 'before start' position


This does not look right. Past-end is a valid index, before-start is not. Valid indices can only be in the following range [startIndex, endIndex).

Yes. In addition to the comment being misleading, this specific test is really confusingly written and I will redo it to make the intent clearer.

austinzheng · 2016-06-14T19:45:16Z

I looked into that test more closely. The test was correct, but written in a very confusing way, so I changed it around and made the preconditions it tests explicit in a comment. The "before start index" comment was simply wrong and I've removed it.

I'm happy to make any other changes you'd like. Also let me know if you want me to squash the commit. @moiseev

Changes: - Implemented O(1) `index(_:offsetBy:)` and `distance(from:to:)` methods on `CountableClosedRange` - Fixed some unit tests - Added many more unit tests for Countable*Range index APIs

moiseev · 2016-06-16T16:51:14Z

@swift-ci Please test

austinzheng · 2016-06-16T20:29:02Z

Looks like everything's in working order.

gribozavr · 2016-06-17T04:10:15Z

stdlib/public/core/ClosedRange.swift

@@ -47,26 +47,64 @@ public struct ClosedRangeIndex<Bound> : Comparable
    }
  }

-  internal func _predecessor(upperBound limit: Bound) -> ClosedRangeIndex {
+  internal func _predecessor(
+    lowerBound: Bound, upperBound limit: Bound


At this point, now that you are passing both bounds, I think it would make sense to just move this code into its only callsite.

moiseev · 2016-06-17T04:15:22Z

Ooops. Sorry, @gribozavr, I merged it before seeing your comments.

gribozavr · 2016-06-17T04:15:35Z

NP.

gribozavr · 2016-06-17T04:19:24Z

validation-test/stdlib/Range.swift.gyb

+
+  /// Return the index `leftIndex` steps away from `start`, or the collection's
+  /// `endIndex` if `leftIndex` is `nil`.
+  func leftIndex<C: RandomAccessCollection>(forCollection c: C) -> C.Index {


Please use just for: for the label (Collection is redundant).

Or even of:.

gribozavr · 2016-06-17T04:29:10Z

@austinzheng Great work! 👍

austinzheng · 2016-06-17T04:47:16Z

Thanks! I'll open a new PR to address these comments soon.

On Jun 16, 2016, at 9:30 PM, Dmitri Gribenko notifications@github.com wrote:

@austinzheng https://github.com/austinzheng Great work! 👍

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #2993 (comment), or mute the thread https://github.com/notifications/unsubscribe/ADhCjj2Oa2_JeiEXJMX6gooouuKqnb9nks5qMiLdgaJpZM4IzteZ.

Changes: - Moved only-used-once helper methods into call sites - Parameterized tests that might expect a crash - Fixed some comments and formatting; renamed some test helper APIs

austinzheng force-pushed the az-rangework branch from 98c3593 to 2fc3ccd Compare June 12, 2016 05:54

moiseev changed the title ~~Implementing O(1) methods on CountableClosedRange~~ [stdlib] Implementing O(1) methods on CountableClosedRange Jun 13, 2016

moiseev reviewed Jun 13, 2016
View reviewed changes

moiseev self-assigned this Jun 13, 2016

austinzheng force-pushed the az-rangework branch from 2fc3ccd to 52d61ec Compare June 14, 2016 01:11

austinzheng force-pushed the az-rangework branch 3 times, most recently from 3c6aa17 to 49241da Compare June 14, 2016 04:52

austinzheng force-pushed the az-rangework branch 2 times, most recently from e0d164e to c8e5228 Compare June 14, 2016 05:57

moiseev reviewed Jun 14, 2016
View reviewed changes

austinzheng force-pushed the az-rangework branch 5 times, most recently from df1da50 to 5d955f1 Compare June 16, 2016 07:53

austinzheng added 2 commits June 16, 2016 09:03

[stdlib] Implementing O(1) methods on CountableClosedRange

d261e7b

Changes: - Implemented O(1) `index(_:offsetBy:)` and `distance(from:to:)` methods on `CountableClosedRange` - Fixed some unit tests - Added many more unit tests for Countable*Range index APIs

Clarify test intent

59789d3

austinzheng force-pushed the az-rangework branch from 5d955f1 to 59789d3 Compare June 16, 2016 16:03

moiseev merged commit 6231cca into swiftlang:master Jun 17, 2016

gribozavr reviewed Jun 17, 2016
View reviewed changes

austinzheng mentioned this pull request Jun 17, 2016

[stdlib] Addressing Dmitri Gribenko's feedback re #2993 #3047

Merged

1 task

austinzheng deleted the az-rangework branch June 19, 2016 04:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stdlib] Implementing O(1) methods on `CountableClosedRange` #2993

[stdlib] Implementing O(1) methods on `CountableClosedRange` #2993

austinzheng commented Jun 12, 2016 •

edited

Loading

moiseev commented Jun 13, 2016

moiseev Jun 13, 2016

moiseev commented Jun 13, 2016

austinzheng commented Jun 13, 2016

austinzheng commented Jun 14, 2016

austinzheng commented Jun 14, 2016 •

edited

Loading

austinzheng commented Jun 14, 2016

moiseev Jun 14, 2016

austinzheng Jun 14, 2016 •

edited

Loading

austinzheng commented Jun 14, 2016

moiseev commented Jun 16, 2016

austinzheng commented Jun 16, 2016

gribozavr Jun 17, 2016

moiseev commented Jun 17, 2016

gribozavr commented Jun 17, 2016

gribozavr Jun 17, 2016

gribozavr Jun 17, 2016

gribozavr commented Jun 17, 2016

austinzheng commented Jun 17, 2016

[stdlib] Implementing O(1) methods on CountableClosedRange #2993

[stdlib] Implementing O(1) methods on CountableClosedRange #2993

Conversation

austinzheng commented Jun 12, 2016 • edited Loading

What's in this pull request?

moiseev commented Jun 13, 2016

moiseev Jun 13, 2016

Choose a reason for hiding this comment

moiseev commented Jun 13, 2016

austinzheng commented Jun 13, 2016

austinzheng commented Jun 14, 2016

austinzheng commented Jun 14, 2016 • edited Loading

austinzheng commented Jun 14, 2016

moiseev Jun 14, 2016

Choose a reason for hiding this comment

austinzheng Jun 14, 2016 • edited Loading

Choose a reason for hiding this comment

austinzheng commented Jun 14, 2016

moiseev commented Jun 16, 2016

austinzheng commented Jun 16, 2016

gribozavr Jun 17, 2016

Choose a reason for hiding this comment

moiseev commented Jun 17, 2016

gribozavr commented Jun 17, 2016

gribozavr Jun 17, 2016

Choose a reason for hiding this comment

gribozavr Jun 17, 2016

Choose a reason for hiding this comment

gribozavr commented Jun 17, 2016

austinzheng commented Jun 17, 2016

[stdlib] Implementing O(1) methods on `CountableClosedRange` #2993

[stdlib] Implementing O(1) methods on `CountableClosedRange` #2993

austinzheng commented Jun 12, 2016 •

edited

Loading

austinzheng commented Jun 14, 2016 •

edited

Loading

austinzheng Jun 14, 2016 •

edited

Loading