Skip to content

jextract: Remove $ from generated method parameters#581

Merged
ktoso merged 1 commit intoswiftlang:mainfrom
amanthatdoescares:fix/remove-dollar-from-params
Mar 3, 2026
Merged

jextract: Remove $ from generated method parameters#581
ktoso merged 1 commit intoswiftlang:mainfrom
amanthatdoescares:fix/remove-dollar-from-params

Conversation

@amanthatdoescares
Copy link
Contributor

@amanthatdoescares amanthatdoescares commented Mar 3, 2026

Fixes #494

Generated code was using swiftArena$ as the parameter name for the arena. The $ suffix is meant for names inside method bodies (e.g. arena$, ex$) to avoid clashing with real identifiers, not for public parameters. This PR changes the generated parameter to plain swiftArena everywhere.

Updates are in the FFM and JNI generators

  1. FFMSwift2JavaGenerator+JavaBindingsPrinting.swift,
  2. FFMSwift2JavaGenerator+JavaTranslation.swift,
  3. JNISwift2JavaGenerator+JavaBindingsPrinting.swift,
  4. JNISwift2JavaGenerator+JavaTranslation.swift

in TranslatedDocumentation.swift for Javadoc, and in the tests and sample apps that assert or use this API.

Names like arena$ and ex$ inside method bodies are left as-is.

swift test --filter JExtractSwiftTests passes.

@amanthatdoescares amanthatdoescares requested a review from ktoso as a code owner March 3, 2026 12:34
@ktoso
Copy link
Collaborator

ktoso commented Mar 3, 2026

To what extent was this PR prepared by automated tools? I am concerned given the shape of the opening message.

@amanthatdoescares
Copy link
Contributor Author

To what extent was this PR prepared by automated tools? I am concerned given the shape of the opening message.

I used an AI coding assistant to implement this: it searched the repo for swiftArena$, proposed the edits across the generators, docs, tests, and samples, and I reviewed and committed them. I also ran the JExtractSwiftTests locally to confirm everything passes. The PR description was drafted with the assistant; I’m happy to rephrase it if you’d prefer a different style.

@ktoso
Copy link
Collaborator

ktoso commented Mar 3, 2026

Thank you for clarifying.

We don't have an official policy yet but yes please, it would be helpful to read messages written by humans summarizing a change. It's a lot of text to read through and didn't make me very confident in the change.

It's a pretty small modification and we don't mind assisted PRs but it would be preferable if you explained how it was used or what a PR does using your own voice, thanks in advance.

Let's see if this covered all cases, I'm ok merging it if tests pass 👍

@ktoso
Copy link
Collaborator

ktoso commented Mar 3, 2026

Well, seems it did a good job, happy accepting the change -- please write PR message yourself or summarize what the tool provided, that'll help me review changes.

Thank you!

@ktoso ktoso merged commit 409e0d0 into swiftlang:main Mar 3, 2026
50 checks passed
@amanthatdoescares
Copy link
Contributor Author

Well, seems it did a good job, happy accepting the change -- please write PR message yourself or summarize what the tool provided, that'll help me review changes.

Thank you!

Thank you,
You’re right, I should have summarized the change more clearly.

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.

jextract: Remove $ from generated method parameters

2 participants