SR-7266: Avoid nesting of ReversedCollection #15815

Moximillian · 2018-04-07T15:29:31Z

Applying reversed() two times should return original collection.

First round of fixes for the SR-7266, to avoid nesting multiple ReversedCollections and instead return the original collection with two consecutive .reversed() functions.

Does not yet fix the .lazy use case.

Resolves SR-7266.

danielctull · 2018-04-07T17:38:17Z

Hi there, I started looking at this bug last weekend with a solution to the lazy scenario, which you can find at this gist: https://gist.github.com/danielctull/ec521f3fc2aadabb17d38f716e55c5f7

It seems tricky to constrain LazyCollectionProtocol in a way to overload a version of the reversed() function when the lazy's base is itself a ReversedCollection. I've been meaning to raise this as a PR, to see whether the additional protocol would justified for use only in this scenario.

Moximillian · 2018-04-07T18:35:33Z

Hi, yes, I also ended up with having to create ReversedCollectionProtocol for the lazy case. It's now updated to the PR.

Moximillian · 2018-04-07T18:46:35Z

Would
extension LazyCollectionProtocol where Self: BidirectionalCollection, Elements: ReversedCollectionProtocol {}
be preferable over
extension LazyCollection where Base: ReversedCollectionProtocol {}
?

moiseev

@Moximillian, thanks for addressing the issue!

AFAIK @danielctull has tests and properly set availability attributes for the old and new reversed overloads. It would make a lot of sense for you two to merge efforts.

Let's concentrate on a non-lazy case first.

moiseev · 2018-04-09T16:46:53Z

stdlib/public/core/Reverse.swift

@@ -253,6 +253,19 @@ extension ReversedCollection: BidirectionalCollection {

 extension ReversedCollection: RandomAccessCollection where Base: RandomAccessCollection { }

+extension ReversedCollection {
+  /// This is optimization to return identity of doubly reversed collection


This part of the comment is an implementation detail, I don't think it should be a doc-comment.

moiseev · 2018-04-09T16:54:45Z

stdlib/public/core/Reverse.swift

+  // For example [1,2].reversed().reversed() => [1,2]
+
+  /// Returns the elements of the original collection (reversal of reversal)
+  public func reversed() -> Base {


Since this is a more concrete overload of an existing method, we should be careful not to break existing code.

The type of let xs = [1,2,3].reversed().reversed() changes. This new overload should be @available(swift, introduced: 4.2).

danielctull · 2018-04-09T17:54:59Z

As @moiseev said, I have a branch which adds tests and availability attributes which you can see in this comparison.

Moximillian · 2018-04-09T21:08:06Z

My first starterbug so please bear with me :)

@moiseev:

I amended the comments to align with @danielctull
added availability and inline
added the testcase for non-lazy variant from @danielctull
I don't know if you meant this, but the PR now only contains the non-lazy case, which means it does not add or use the new ReversedCollectionProtocol at all.

danielctull · 2018-04-10T12:18:59Z

Hi @Moximillian! I've sent you a more long winded email but to paraphrase here: Because I'm a little vain and I'd like to be able to have recognition for my work, I split my test commit into the regular and lazy checks and rebased that into your branch. You can see this here: https://github.com/danielctull-forks/swift/commits/SR-7266-avoid-nesting-reversed. It keeps all of you other changes, but allows me to be the committer for the test additions.

You could either reset my branch or cherry-pick my commit dbdd74a8a8fcf6f70cc9810fe81b5efaa18bd57e and force push to your fork to update this PR.

Applying reversed() two times should return identity.

Moximillian · 2018-04-10T14:20:06Z

Thanks @danielctull, squashed my commits and cherry-picked your test commit. Git is amazing tool, interesting to learn more about it :)

So all-in-all, this is now same as before: just adding the non-lazy optimisation for ReversedCollection + tests for it.

danielctull · 2018-04-10T15:16:38Z

Cool, cheers @Moximillian. I think this line to run the test was lost when I split out the lazy tests earlier, if you want to merge this one in too: 110655befd06952c82f4f28406476da573962d8c

I ran these on the previous branch (before you squashed) and it ran fine! 😁

Moximillian · 2018-04-10T16:29:50Z

Cherry-picked the missing line as well

moiseev · 2018-04-10T21:22:40Z

stdlib/public/core/Reverse.swift

@@ -84,7 +82,7 @@ extension ReversedCollection {
    }
  }
 }
- 
+


nitpick: Unnecessary space at the end of the line?

so my change removes the unnecessary space from that line. Or would you rather not have those kind of cleanups in this PR?

Oh. I misinterpreted the diff. I would prefer to not mix the cleanups with functional changes, but am guilty of doing it every now and then myself. So let's keep it.

moiseev · 2018-04-11T00:08:05Z

@swift-ci Please test

moiseev · 2018-04-11T00:08:23Z

@swift-ci Please Test Source Compatibility

moiseev · 2018-04-12T22:05:29Z

For the record. The binary size difference is negligible.

moiseev reviewed Apr 9, 2018

View reviewed changes

SR-7266: Avoid nesting of ReversedCollection

b859cdc

Applying reversed() two times should return identity.

Moximillian force-pushed the SR-7266-avoid-nesting-reversed branch from 768c1c3 to b859cdc Compare April 10, 2018 14:06

[stdlib] SR-7266 Add ReverseCompatibility test

9c13a27

[stdlib] SR-7266 Make sure to actually run the tests

4714952

moiseev reviewed Apr 10, 2018

View reviewed changes

moiseev merged commit 02aef12 into swiftlang:master Apr 12, 2018

Moximillian deleted the SR-7266-avoid-nesting-reversed branch April 16, 2018 22:01

Moximillian mentioned this pull request Jun 7, 2018

DO NOT MERGE [stdlib] SR 7266 avoid nesting reversed lazy #17052

Closed

Moximillian mentioned this pull request Apr 7, 2018

[SR-7266] reversed() applied multiple times should not nest ReversedCollection into itself #49814

Closed

@@ @@ -84,7 +82,7 @@ extension ReversedCollection { @@
                   }
                 }
               }

SR-7266: Avoid nesting of ReversedCollection #15815

SR-7266: Avoid nesting of ReversedCollection #15815

Uh oh!

Conversation

Moximillian commented Apr 7, 2018

Uh oh!

danielctull commented Apr 7, 2018

Uh oh!

Moximillian commented Apr 7, 2018

Uh oh!

Moximillian commented Apr 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moiseev left a comment

Choose a reason for hiding this comment

Uh oh!

moiseev Apr 9, 2018

Choose a reason for hiding this comment

Uh oh!

moiseev Apr 9, 2018

Choose a reason for hiding this comment

Uh oh!

danielctull commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Moximillian commented Apr 9, 2018

Uh oh!

danielctull commented Apr 10, 2018

Uh oh!

Moximillian commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielctull commented Apr 10, 2018

Uh oh!

Moximillian commented Apr 10, 2018

Uh oh!

moiseev Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

Moximillian Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

moiseev Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

moiseev commented Apr 11, 2018

Uh oh!

moiseev commented Apr 11, 2018

Uh oh!

moiseev commented Apr 12, 2018

Uh oh!

Uh oh!

Moximillian commented Apr 7, 2018 •

edited

Loading

danielctull commented Apr 9, 2018 •

edited

Loading

Moximillian commented Apr 10, 2018 •

edited

Loading