[NFC][ASTGen] Add wrapping structs for commonly bridged types #66044
Conversation
bnbarham
requested review from
zoecarver,
CodaFi,
hborla,
slavapestov and
xedin
as code owners
|
@swift-ci please test |
|
@swift-ci please test |
| Diagnostic_create( | ||
| cxxDiagnosticEngine, bridgedSeverity, | ||
| cxxSourceLocation(at: position), | ||
| messageBuffer.baseAddress, messageBuffer.count) | ||
| bridgedSourceLoc(at: position), | ||
| bridgedMessage) |
There was a problem hiding this comment.
Note to myself: We should also run swift-format on the ASTGen sources, I think.
There was a problem hiding this comment.
Yeah, that'd be nice.
There was a problem hiding this comment.
Note to myself: We should also run
swift-formaton theASTGensources, I think.
I’ve been doing some progress on ASTGen the past few days as well and I figured to borrow the .swift-format file from swift-syntax with a line length adjusted to 100. Do we want it to be 80 as in the Standard Library?
There was a problem hiding this comment.
Just for the record: I very much like not having a maximum line length in swift-syntax because in my experience the line length limitation sometimes causes us choose less descriptive, shorter variable names. And in my opinion it hasn’t lead to excessively long lines.
Also, just to make an informed decision, here’s a chart of the line lengths in non-generated SwiftSyntax code.
Some key data points:
- ≤160 characters: 99.9%
- ≤150 characters: 99.8%
- ≤140 characters: 99.7%
- ≤130 characters: 99.5%
- ≤120 characters: 99.1%
- ≤110 characters: 98.6%
- ≤100 characters: 97.9%
- ≤90 characters: 96.7%
- ≤80 characters: 94.4%
Also, I think that the format (and thus the maximum line length) should be consistent between swift-syntax and ASTGen.
So: If we want to impose a maximum line length (and just to be clear, I like the freedom of not having a maximum line length but the general consensus is that people want a restriction here), I would like to pick one for swift-syntax and then also use that for ASTGen.
I just did some tests on swift-syntax and all changes for the line length to 160 seemed reasonable to me. Dropping it to 150 already gave me a couple cases where I preferred the previous formatting. The trend continue as I dropped it further.
There was a problem hiding this comment.
Thanks for the visuals!
Also, I think that the format (and thus the maximum line length) should be consistent between swift-syntax and ASTGen.
Sure, I generally think having a single format across swift-syntax and Swift parts of the compiler would be great.
I just did some tests on swift-syntax and all changes for the line length to 160 seemed reasonable to me. Dropping it to 150 already gave me a couple cases where I preferred the previous formatting. The trend continue as I dropped it further.
I guess it has to be a compromise based on what line length reasonably fits on most of our screens without wrapping. Either way I do believe we need some kind of limit below 200 that we can use and refer to in reviews or else this is bound to get out of control. Personally, I probably wouldn’t be fine with anything more than 160.
There was a problem hiding this comment.
Something is better than nothing, so if that's 160 I'm personally fine with that :)
bnbarham
force-pushed
the
cleanup-astgen
branch
3 times, most recently
from
3e7af98 to
0cb2560
Compare
bnbarham
changed the title
|
@swift-ci please test |
|
@swift-ci please test macOS platform |
|
@swift-ci please test Linux platform |
Adds wrapping structs for a bunch of commonly bridged types (but not all the AST nodes yet).
|
@swift-ci please test |
|
@swift-ci please test macOS platform |
Adds wrapping structs for a bunch of commonly bridged types (but not all the AST nodes yet).