Skip to content
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

Add a custom @_spi kind to expose BumpPtrAllocator from SwiftSyntax to ASTGen. #68351

Closed
ahoppen opened this issue Sep 6, 2023 · 15 comments · Fixed by #68860 · May be fixed by #68845
Closed

Add a custom @_spi kind to expose BumpPtrAllocator from SwiftSyntax to ASTGen. #68351

ahoppen opened this issue Sep 6, 2023 · 15 comments · Fixed by #68860 · May be fixed by #68845
Assignees
Labels
ASTGen Area → compiler: The ASTGen module, which translates SwiftSyntax trees to the C++ AST compiler The Swift compiler itself good first issue Good for newcomers task

Comments

@ahoppen
Copy link
Member

ahoppen commented Sep 6, 2023

Mark BumpPtrAllocator as @_spi(BumpPtrAllocator) and change @_spi(RawSyntax) import SwiftSyntax in ASTGen.swift to @_spi(BumpPtrAllocator) import SwiftSyntax.

That way, ASTGen doesn’t need to import SwiftSyntax with @_spi(RawSyntax).

@ahoppen ahoppen added the ASTGen Area → compiler: The ASTGen module, which translates SwiftSyntax trees to the C++ AST label Sep 6, 2023
@AnthonyLatsis AnthonyLatsis added compiler The Swift compiler itself task good first issue Good for newcomers labels Sep 8, 2023
@ChiduAnush
Copy link
Contributor

hello, I am a total newbie to open source. Would be great if I could start woking on this. just confirming, i just need to find and replace right?.
thank you

@ahoppen
Copy link
Member Author

ahoppen commented Sep 14, 2023

Yes, should be quite straightforward. Just note that the apple/swift side of this issue depends on #67111 being merged.

@ChiduAnush
Copy link
Contributor

what does that mean? sorry if this is a very basic doubt. should I fork the repo and start to work now then? or wait?

@ahoppen
Copy link
Member Author

ahoppen commented Sep 14, 2023

I think it’s probably easiest if you just wait until #67111 is merged.

@ChiduAnush
Copy link
Contributor

ah that's great. thank you so much. will keep an eye out

@veryharsh123
Copy link

Can I work on the issue?

@AnthonyLatsis
Copy link
Collaborator

@ChiduAnush Are you still keen to work on this?

@ChiduAnush
Copy link
Contributor

Yes, would be great if you could assign it

@AnthonyLatsis
Copy link
Collaborator

Yes, would be great if you could assign it

You're already assigned!


@veryharsh123 Do you mind deferring this issue to @ChiduAnush since he was already assigned?

@ChiduAnush
Copy link
Contributor

can you tell me what exactly do you mean by Mark BumpPtrAllocator as @_spi(BumpPtrAllocator)?

@AnthonyLatsis
Copy link
Collaborator

Sure. swift/main depends on swift-syntax/main, so you need 2 simultaneous pull requests to pull this off: one in https://github.com/apple/swift-syntax that marks the BumpPtrAllocator class as @_spi(BumpPtrAllocator) instead of @_spi(RawSyntax), and a second one in this repository that updates the import declaration in ASTGen.swift.

@ChiduAnush
Copy link
Contributor

please check and let me know if everything is all right.

@ahoppen
Copy link
Member Author

ahoppen commented Sep 30, 2023

I think you also need to change the import of SwiftSyntax in ASTGen.swift to be @_spi(BumpPtrAllocator)

https://github.com/apple/swift/blob/95330bcc5bf95a1bf1ff3a00d71d701101af57eb/lib/ASTGen/Sources/ASTGen/ASTGen.swift#L7

@ChiduAnush
Copy link
Contributor

I already did that. There are 2 pull requests.

@ahoppen
Copy link
Member Author

ahoppen commented Oct 2, 2023

Oh, sorry. I didn’t see your PR to apple/swift. In general, if you have two PRs that are linked like that, it’s good practice to reference the accompanying PR in the other repo in the PR’s description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASTGen Area → compiler: The ASTGen module, which translates SwiftSyntax trees to the C++ AST compiler The Swift compiler itself good first issue Good for newcomers task
Projects
None yet
4 participants