close
Skip to content

Treat comments as whitespace for operator arity rules, implementing SE-0037#1732

Merged
lattner merged 4 commits intoswiftlang:masterfrom
jder:comment-operator-fixes
Apr 3, 2016
Merged

Treat comments as whitespace for operator arity rules, implementing SE-0037#1732
lattner merged 4 commits intoswiftlang:masterfrom
jder:comment-operator-fixes

Conversation

@jder
Copy link
Copy Markdown
Contributor

@jder jder commented Mar 18, 2016

What's in this pull request?

Implements SE-0037, "Clarify interaction between comments & operators" by treating comments as whitespace in isRightBound and isLeftBound, and fixing up right-boundedness after splitting a token at a comment start.

Also removes a test which can no longer parse. I can't see the underlying radar ( rdar://problem/18662272), so if someone at Apple could take a look and see if there's an alternative to just deleting it wholesale, that'd be great.

Resolved bug number: (SR-186, SR-960)


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.

jder added 2 commits March 18, 2016 11:51
Previously, comments were treated as non-whitespace. Operators
also checked for right-boundedness before detecting comments
which start in the middle of an operator.

This resolves both of these issues so that comments are
consistently treated as whitespace when determining whether
operators are left- or right-bound.

Fixes SR-186 and SR-960 (SE-0037)
@gribozavr
Copy link
Copy Markdown
Contributor

Not a review, just triggering the CI.

@swift-ci Please test

@tkremenek
Copy link
Copy Markdown
Member

@swift-ci Please test

@tkremenek
Copy link
Copy Markdown
Member

@DougGregor Please review.

@tkremenek
Copy link
Copy Markdown
Member

@lattner Please review.

Comment thread lib/Parse/Lexer.cpp Outdated
return false;
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic looks correct, but for better efficiency, please put this into the default case in the switch below. That way it isn't run in the common case where a token is preceded by whitespace.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've moved it to a separate case, which I think reads better than in the default case since it also depends on tokBegin[-1].

@lattner
Copy link
Copy Markdown
Contributor

lattner commented Mar 28, 2016

Looking good, just a few comments on the patch.

@jder
Copy link
Copy Markdown
Contributor Author

jder commented Apr 2, 2016

Thanks for the review, @lattner! I think this is ready for another look.

@lattner lattner merged commit 770b292 into swiftlang:master Apr 3, 2016
@lattner
Copy link
Copy Markdown
Contributor

lattner commented Apr 3, 2016

Looks great! Thank you again. Please send a PR for the changelog to mention this (referencing the SE) and a PR for the swift-evolution repo to move it into the "implemented" category. If you'd prefer me to take care of this, just let me know. Thanks again!

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.

5 participants