Skip to content

[jnigen] Make Visitor constructable inline - #3521

Open
liamappelbe wants to merge 7 commits into
mainfrom
jnigen_visitor
Open

[jnigen] Make Visitor constructable inline#3521
liamappelbe wants to merge 7 commits into
mainfrom
jnigen_visitor

Conversation

@liamappelbe

Copy link
Copy Markdown
Contributor

This pattern was suggested for the new FFIgen visitor API, but it would also be handy to have on the JNIgen API.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Package publishing

If you have publishing permissions, you can use the links below to publish the changes after merging this PR.

Package Version Status Publish tag (post-merge)
package:code_assets 1.3.0-wip WIP (no publish necessary)
package:data_assets 0.20.0 already published at pub.dev
package:ffi 2.2.0 already published at pub.dev
package:hooks 2.2.0-wip WIP (no publish necessary)
package:hooks_runner 1.6.2-wip WIP (no publish necessary)
package:jni_flutter 1.0.2 already published at pub.dev
package:jni_util 1.0.0 already published at pub.dev
package:native_toolchain_c 0.19.4-wip WIP (no publish necessary)
package:record_use 1.1.0 ready to publish record_use-v1.1.0
package:swift2objc 0.3.0-wip WIP (no publish necessary)
package:swiftgen 0.1.3 already published at pub.dev

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbol Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
jni None 1.0.3 1.0.3 1.0.3 ✔️

This check can be disabled by tagging the PR with skip-breaking-check.

abstract class Visitor {
const Visitor();

factory Visitor.callback({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's unfortunate this has to be called callback.

Does it work to just call it Visitor(...)? I guess not because the subclasses written by users need to call the parent constructor and it's weird if that one cannot be called Visitor(...).

How about Visitor.inline() or Visitor.fromCallbacks(...), Visitor.concrete(...), Visitor.implementation(...)

Or we should consider making the default constructor not the default name. E.g. that makes the inline visitors more pretty and the custom subclasses slightly less pretty.

class MyVisitor extends Visitor
  MyVisitor(...):super.base();

Side note. I feel we should make a decision whether we want users to subclass or implement visitor. It should either be a base class (non-implementable) or interface class (non-extendible).

If it's only implementable and not extendible, then we don't need a default constructor for sub classes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The downside of making it only implementable is that users have to override every method. That's not a big deal for JNIgen's visitors, but FFIgen will have heaps of visit methods. I've gone with the super.base() option.

@liamappelbe
liamappelbe requested a review from dcharkes August 6, 2026 03:31
@@ -15,6 +15,15 @@ abstract class _Element {
/// Users can extend this class to create custom visitors that modify the AST
/// before code generation.
abstract class Visitor {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

abstract base class

@goderbauer goderbauer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants