Skip to content

Refactor Syntax initializer #1092

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
Dec 15, 2022
Merged

Refactor Syntax initializer #1092

merged 1 commit into from
Dec 15, 2022
+3,058 −5,917

Conversation

bnbarham
Copy link
Contributor

@bnbarham bnbarham commented Nov 24, 2022 •
edited
Loading

Use generics rather than existentials, add nil defaults for all
optional nodes, and add a default for tokens where there's only a single
choice. Allows removing the default initializer in SwiftSyntaxBuilder,
since the regular initializer covers it entirely.

There's a small hack that adds a second initializer when a parameter
with a generic type has a empty default, since a
Optional<BaseType>.none default doesn't work in Swift 5.6. This can be
removed and the property initializer updated once 5.7 is our minimum
version.

@bnbarham bnbarham requested a review from ahoppen as a code owner November 24, 2022 03:59
bnbarham
bnbarham commented Nov 24, 2022
View reviewed changes
@bnbarham bnbarham force-pushed the add-syntax-init-defaults branch from 0841315 to 37c8e1d Compare November 24, 2022 06:07
ahoppen
ahoppen reviewed Nov 24, 2022
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.

Thanks a lot, this is a great simplification 🙏

Do you know if the generic initializers affect code size or performance in either direction?

bnbarham
bnbarham commented Nov 24, 2022
View reviewed changes
Copy link
Contributor Author

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

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

I can check performance/size next week

@bnbarham bnbarham force-pushed the add-syntax-init-defaults branch 2 times, most recently from 2f904b1 to 8cbc881 Compare November 25, 2022 20:48
@bnbarham bnbarham force-pushed the add-syntax-init-defaults branch from 8cbc881 to 93a6d61 Compare December 3, 2022 23:21
@bnbarham
Copy link
Contributor Author

bnbarham commented Dec 3, 2022 •
edited
Loading

I can check performance/size next week

We don't really have any infrastructure for testing syntax building performance at the moment. There's no changes to Raw* so unsurprisingly there's no real parsing difference. Sizes have dropped slightly:

                            Pre      Post
libIDEUtils.a               32016096 31767448
libSwiftDiagnostics.a       32158792 31910144
libSwiftOperators.a         41534824 41288040
libSwiftParser.a            40506392 40257752
libSwiftParserDiagnostics.a 42527096 42279544
libSwiftRefactor.a          40836936 40589096
libSwiftSyntax.a            31890912 31642272
libSwiftSyntaxBuilder.a     48013032 46971680
libSwiftSyntaxParser.a      40854664 40606016
lib_SwiftSyntaxMacros.a     48306464 47265344

@bnbarham
Copy link
Contributor Author

bnbarham commented Dec 3, 2022

@swift-ci please test

ahoppen
ahoppen approved these changes Dec 4, 2022
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.

Thanks for checking performance and size.

@bnbarham bnbarham force-pushed the add-syntax-init-defaults branch from 93a6d61 to eda8373 Compare December 6, 2022 21:14
@bnbarham
Copy link
Contributor Author

bnbarham commented Dec 6, 2022

@swift-ci please test

@bnbarham bnbarham force-pushed the add-syntax-init-defaults branch from eda8373 to 71d4767 Compare December 7, 2022 01:26
@bnbarham
Copy link
Contributor Author

bnbarham commented Dec 7, 2022

@swift-ci please test

bnbarham
bnbarham commented Dec 7, 2022
View reviewed changes
@bnbarham bnbarham force-pushed the add-syntax-init-defaults branch 2 times, most recently from 1bd363c to 9a4e8a9 Compare December 8, 2022 01:42
@bnbarham
Copy link
Contributor Author

bnbarham commented Dec 8, 2022

@swift-ci please test

@bnbarham bnbarham force-pushed the add-syntax-init-defaults branch from 9a4e8a9 to 8e9bf5c Compare December 8, 2022 21:11
@bnbarham
Copy link
Contributor Author

bnbarham commented Dec 8, 2022

@swift-ci please test

@bnbarham bnbarham force-pushed the add-syntax-init-defaults branch 2 times, most recently from 88ca6bb to b6c9ea4 Compare December 10, 2022 19:03
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham force-pushed the add-syntax-init-defaults branch from b6c9ea4 to ef5a12c Compare December 12, 2022 19:36
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham force-pushed the add-syntax-init-defaults branch from ef5a12c to f8830ef Compare December 13, 2022 17:19
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham force-pushed the add-syntax-init-defaults branch from f8830ef to 4f4fcdc Compare December 13, 2022 21:15
@bnbarham
Copy link
Contributor Author

@swift-ci please test

ahoppen
ahoppen approved these changes Dec 14, 2022
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’ve reviewed the entire PR again and have one more comment. Otherwise, let’s 🚢it

@bnbarham
Refactor Syntax initializer
e71b64a 
Use generics rather than existentials, add `nil` defaults for *all*
optional nodes, and add a default for tokens where there's only a single
choice. Allows removing the default initializer in SwiftSyntaxBuilder,
since the regular initializer covers it entirely.

There's a small hack that adds a second initializer when a parameter
with a generic type has a empty default, since a
`Optional<BaseType>.none` default doesn't work in Swift 5.6. This can be
removed and the property initializer updated once 5.7 is our minimum
version.
@bnbarham bnbarham force-pushed the add-syntax-init-defaults branch from 4f4fcdc to e71b64a Compare December 14, 2022 21:18
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham merged commit 378dd9c into swiftlang:main Dec 15, 2022
@bnbarham bnbarham deleted the add-syntax-init-defaults branch December 15, 2022 01:16
@ahoppen ahoppen mentioned this pull request Dec 15, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen approved these changes

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

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

@CodaFi CodaFi Awaiting requested review from CodaFi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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