Skip to content

[reference-bindings] Add support for borrow, inout bindings. #1363

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

Conversation

gottesmm
Copy link
Contributor

To make sure that I did not stomp on anything already working around borrow, inout, I decided to (for now) use the keywords inoutBindingKW and borrowBindingKW, just to be careful.

@gottesmm gottesmm requested a review from ahoppen as a code owner February 27, 2023 01:48
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

@ahoppen do I need to update anything else?

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

What is stopping you from just using borrow and inout as keywords here? If you want to introduce this as a staged process with new keywords for now, I think it would make sense to underscore borrowBindingKW and inoutBindingKW to indicate they aren’t the final design.

@gottesmm gottesmm force-pushed the pr-56d5f24cde74ce112ffcd40bc4c23e08e70aae99 branch from a1896bd to db1a31c Compare February 27, 2023 18:51
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm gottesmm force-pushed the pr-56d5f24cde74ce112ffcd40bc4c23e08e70aae99 branch from db1a31c to 306ac7b Compare February 27, 2023 19:15
@gottesmm
Copy link
Contributor Author

@swift-ci test

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

One improvement suggestion for a comment, otherwise LGTM.

Comment on lines +195 to +197
//
// NOTE: We reuse this for inout bindings and choose the higher precedence level of expr keywords
// so we do not break anything.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//
// NOTE: We reuse this for inout bindings and choose the higher precedence level of expr keywords
// so we do not break anything.
//
// NOTE: We reuse this for inout bindings and choose the lower precedence level of expr keywords
// so we don’t skip over expression keywords while looking for `inout` in an expression position.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahoppen I am going to do that in a follow on commit. I don't want to restart CI

@gottesmm
Copy link
Contributor Author

@swift-ci test

2 similar comments
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm gottesmm force-pushed the pr-56d5f24cde74ce112ffcd40bc4c23e08e70aae99 branch from 306ac7b to 3b021d7 Compare March 1, 2023 22:22
@gottesmm
Copy link
Contributor Author

gottesmm commented Mar 1, 2023

@swift-ci test

…ypes.

This was exposed by:

decl/enum/enumtest.swift

The problem here is that we are now allowing for inout to be the start of a
swift decl. This means that if one defines an enum case with a inout parameter,
the parser gets confused and doesnt think it is part of the type. I worked
around this by changing canParseTupleTypeBody to say that an inout binding
cannot be parsed as part of a tuple type. This fits already with how we want to
model this.
@gottesmm
Copy link
Contributor Author

gottesmm commented Mar 2, 2023

swiftlang/swift#64012

@swift-ci test

@gottesmm
Copy link
Contributor Author

gottesmm commented Mar 2, 2023

swiftlang/swift#64012

@swift-ci test

@gottesmm
Copy link
Contributor Author

gottesmm commented Mar 2, 2023

swiftlang/swift#64012

@swift-ci test

@gottesmm
Copy link
Contributor Author

gottesmm commented Mar 2, 2023

swiftlang/swift#64012

@swift-ci test

@gottesmm gottesmm merged commit 5c870b4 into swiftlang:main Mar 2, 2023
@gottesmm gottesmm deleted the pr-56d5f24cde74ce112ffcd40bc4c23e08e70aae99 branch March 2, 2023 23:16
@gottesmm
Copy link
Contributor Author

gottesmm commented Mar 5, 2023

This is rdar://106262176

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