Skip to content

[ASTGen] Introduce nullable variants of bridging wrappers #69326

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 23, 2023
Merged

[ASTGen] Introduce nullable variants of bridging wrappers #69326

merged 1 commit into from
Oct 23, 2023

Conversation

hamishknight
Copy link
Contributor

@hamishknight hamishknight commented Oct 23, 2023 •
edited
Loading

For nullable nodes, introduce both a non-null and nullable variant of the bridging wrapper. This allows us to annotate the necessary parameters as nullable, but keep the returns of the bridged createParsed methods non-null.

@hamishknight hamishknight added the ASTGen Area → compiler: The ASTGen module, which translates SwiftSyntax trees to the C++ AST label Oct 23, 2023
@hamishknight hamishknight mentioned this pull request Oct 23, 2023
Merged
ahoppen
ahoppen approved these changes Oct 23, 2023
View reviewed changes
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.

I like this design but I’d like to hear @rintaro’s and @bnbarham’s thoughts on this as well.

@hamishknight hamishknight mentioned this pull request Oct 23, 2023
Merged
@hamishknight
[ASTGen] Introduce nullable variants of bridging wrappers
4a4a048 
For nullable nodes, introduce both a non-null and
nullable variant of the bridging wrapper. This
allows us to annotate the necessary parameters
as nullable, but keep the returns of the bridged
`createParsed` methods non-null.
@hamishknight
Copy link
Contributor Author

@swift-ci please test

bnbarham
bnbarham approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@bnbarham bnbarham left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The asNullable is a little unfortunate, but I'm in favor of this given all the places that don't allow nullable but took them anyway.

@hamishknight hamishknight merged commit d1fa767 into swiftlang:main Oct 23, 2023
@hamishknight hamishknight deleted the null-and-void branch October 23, 2023 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnbarham bnbarham bnbarham approved these changes

@ahoppen ahoppen ahoppen approved these changes

@zoecarver zoecarver Awaiting requested review from zoecarver

@CodaFi CodaFi Awaiting requested review from CodaFi CodaFi is a code owner

@hborla hborla Awaiting requested review from hborla hborla is a code owner

@slavapestov slavapestov Awaiting requested review from slavapestov slavapestov is a code owner

@xedin xedin Awaiting requested review from xedin xedin is a code owner

Assignees
No one assigned
Labels
ASTGen Area → compiler: The ASTGen module, which translates SwiftSyntax trees to the C++ AST
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hamishknight @bnbarham @ahoppen