[ffigen] Config update part 3: renaming visitor - #3525
Conversation
Package publishingIf you have publishing permissions, you can use the links below to publish the changes after merging this PR.
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with 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.
This check can be disabled by tagging the PR with |
dcharkes
left a comment
There was a problem hiding this comment.
LGTM
Small nit: Write a general PR description. E.g. We introduce support for renaming in the visitors and all existing renaming logic is pulled out into visitors as an internal implementation strategy.
Also, state that we don't do conflict resolution in a visitor currently. Do we do name conflict resolution before or after the user-defined renames? I suppose we don't change the semantics of in which order we do it. But we should make sure we do something similar for JNIgen and FFIgen and document in the doc comments what happens.
- Conflict resolution before user-defined renames: then conflicting user-renames lead to error messages failing code gen
- User-defined renames before conflict-resolution-renames: then we should emit warnings that a rename happened and might have voided user intend.
| String get originalName => _method.originalName; | ||
|
|
||
| @override | ||
| String get name => _method.symbol.oldName; |
There was a problem hiding this comment.
what are these references to oldName. They are not original name. What does old mean in this context?
| /// Implementations can extend [Visitor] (must call the [Visitor.base] | ||
| /// constructor) or use the [Visitor] factory constructor to provide inline | ||
| /// callbacks for specific nodes. | ||
| abstract base class Visitor { |
There was a problem hiding this comment.
Would benefit from the examples you have on the visitors doc comment already.
(I imagine users would look at the doc comment of the visitor base class more often than the visitors field.)
| const Visitor.base(); | ||
|
|
||
| /// Creates a [Visitor] that delegates visiting to the provided callbacks. | ||
| factory Visitor({ |
There was a problem hiding this comment.
Add a small example snippet in the doc comment.
| rename: (decl) => | ||
| decl.originalName == 'sum' ? 'add' : decl.originalName, | ||
| ), | ||
| visitors: const [_RenamingVisitor()], |
There was a problem hiding this comment.
nit: inline the visitor instead
Add the
Visitorinfra. So far only the renaming logic is ported to visitors, since it's the simplest to port.Related: #2062, #2770, #2830