Skip to content

SR-14221: Add tricky parsing test case for trailing closure #36202

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

Merged
merged 4 commits into from
Mar 4, 2021

Conversation

chenhongjing
Copy link
Contributor

This PR adds a tricky parsing test case to ensure there is a correct interpretation of trailing closure.

Resolves SR-14221

@chenhongjing
Copy link
Contributor Author

@varungandhi-apple Could you please review it? Thanks.

@chenhongjing
Copy link
Contributor Author

@swift-ci please smoke test

@varungandhi-apple
Copy link
Contributor

Hi, thanks for submitting the patch. Couple of points:

  1. I think it would be helpful for this to be in a file with tests for trailing closures, instead of being its own file. I think you can rename test/Parse/multiple_trailing_closures.swift to trailing_closures.swift and add this check there.
  2. New contributors can't trigger CI due to some trouble we've had in the past, but I can do that for you after you're done with making changes.

Comment on lines 3 to 5
func f()->Int{ 42 }

//This should be interpreted as a trailing closure, instead of being interpreted as a computed property with undesired initial value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Please try to keep the spacing and line widths consistent with the rest of the project. This means that comments should have a space after the //, and they need to be wrapped to 80 columns.

For functions, the arrow should have spaces on both sides (and similarly, there needs to be a space before an opening brace).

func f()->Int{ 42 } // right now
func f() -> Int { 42 } // match existing style

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your kind instruction. I've made the changes. A small question--when will I be qualified to trigger CI?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a commit access section on https://swift.org/contributing/#contributing-code which describes the details. Usually commit access and swift-ci access are granted together.

@varungandhi-apple
Copy link
Contributor

@swift-ci smoke test

@varungandhi-apple varungandhi-apple merged commit 58a8b75 into swiftlang:main Mar 4, 2021
@varungandhi-apple
Copy link
Contributor

varungandhi-apple commented Mar 4, 2021

CI is passing; merged! 🚢

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.

2 participants