Skip to content
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.

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

Stride should allow zero size. #2125

Closed
wants to merge 1 commit into from

Conversation

danielgindi
Copy link

What's in this pull request?

People expect strides to allow zero size - like any traditional for loop that would have just skipped.
Adding an additional condition just for that makes the code messier.

But moreover - Xcode itself expects it to allow 0 length stride.
As Xcode automatically converts old for loops to strides, it actually breaks code that may in some cases have zero-length strides.

Example case:
ChartsOrg/Charts#931


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
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
OS X platform @swift-ci Please test OS X platform
Linux platform @swift-ci Please test Linux platform

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

People expect strides to allow zero size - like any traditional `for` loop that would have just skipped.
Adding an additional condition just for that makes the code messier.

But moreover - Xcode itself expects it to allow `0` length stride.
As Xcode automatically converts old `for` loops to strides, it actually **breaks** code that may in some cases have zero-length strides.

ChartsOrg/Charts#931
@tkremenek
Copy link
Member

@swift-ci test

@tkremenek
Copy link
Member

@dabrahams Please review.

@tkremenek
Copy link
Member

@swift-ci test

@gribozavr
Copy link
Contributor

Not a review of the semantics, but if we accept this change, it needs tests.

@tkremenek
Copy link
Member

@danielgindi Please provide tests for this behavior change.

@danielgindi
Copy link
Author

Ok friends, testing is always a good call.

After running some tests, I'm closing this.

I thought that this specific precondition tests for cases where the to is same as from, but it actually checks for interval - which should actually not be zero as it will almost always cause an infinite loop.

@dabrahams
Copy link
Contributor

@danielgindi This may not be the right patch in the end, but your point Xcode migrating for loops has convinced me we a stride function that works for this case.

@danielgindi
Copy link
Author

Yes- some work needs to be done on strides, but we need to carefully look into each case, and there are many of them.

  • The interval may have weird sizes, or zero. (Zero is bad in for-loop also)
  • The to/through may be less then from. There are many cases like these, where the loops should just be skipped. We had a few of them, but I've changed them before I realized that there's something wrong with the fact that the translation is not 1:1.
  • Other cases are definitely out there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants