Migrate to the prototype visitor API - #1
Conversation
|
|
||
| const Sqlite3Visitor([this.nameFilter]); | ||
|
|
||
| bool _matches(String name) { |
There was a problem hiding this comment.
We don't provide any other values right? Would it make sense to inline everything inside the visitor class so that everything is in one place?
| @@ -3,96 +3,96 @@ | |||
|
|
|||
| // dart format off | |||
| const usedSqliteSymbols = { | |||
There was a problem hiding this comment.
does the definition order change with a newer ffigen version?
There was a problem hiding this comment.
At some point I started aggressively sorting AST elements to make the generation more deterministic across machines. That would probably explain this diff.
|
|
||
| @ffi.Native<ffi.Pointer<sqlite3_char>>() | ||
| external ffi.Pointer<sqlite3_char> sqlite3_temp_directory; | ||
| @ffi.Native< |
There was a problem hiding this comment.
Are these only reorderings? I see we added more than we removed? Is there something we can't quite capture with the new API?
There was a problem hiding this comment.
Unclear. I searched for a few of these functions that look added, but all the ones I checked have just been moved. If there are any added symbols that's a bug. We should be able to make the new API behave the same. Maybe gemini can write me an analyzer script to list all the symbols defined in a file.
| @override | ||
| void visitFunc(Func node) { | ||
| if (node.originalName.startsWith('pkg_sqlite3_connection_pool')) { | ||
| node.isExcluded = false; |
There was a problem hiding this comment.
Does this have to be a double-negative? Or could this be node.isIncluded = true?
| if (_structNames.contains(node.originalName)) { | ||
| node.isExcluded = false; | ||
| } else { | ||
| node.isExcluded = true; | ||
| } |
There was a problem hiding this comment.
could just be:
| if (_structNames.contains(node.originalName)) { | |
| node.isExcluded = false; | |
| } else { | |
| node.isExcluded = true; | |
| } | |
| node.IsExcluded = !_structNames.contains(node.originalName); |
But removing the double negative would make this easier to reason about.
| Logger.root.onRecord.listen(print); | ||
|
|
||
| final generator = FfiGenerator( | ||
| visitors: const [Sqlite3ConnectionPoolVisitor()], |
There was a problem hiding this comment.
@dcharkes mentioned this in another place, it is a little unfortunate that configuration is now spread out across two places. Since we don't have anonoumous classes, I like his suggestion of using some sort of base class where you define the visitor callbacks as argument in its constructor.
3 participants
Context: dart-lang/native#3482