Skip to content

[swift2objc] Filtering Support #1730

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

Merged
merged 11 commits into from
Dec 16, 2024

Conversation

nikeokoronkwo
Copy link
Contributor

This Pull Request is to add filtering support to the swiftgen tool.

  • Add filtering support to swiftgen Config
  • Implementing filtering support into transform logic
  • Create and test unit tests for such filtering logic

More information can be found here: #1394

Copy link
Contributor

@liamappelbe liamappelbe left a comment

Choose a reason for hiding this comment

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

Looks like you've also got a merge conflict to resolve.

Copy link
Contributor

@liamappelbe liamappelbe left a comment

Choose a reason for hiding this comment

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

Tests are looking good. You can resolve the formatting and analysis issues in CI using dart format . and dart analyze.

return matches.map((match) => match.group(0)!).toSet();
}

// TODO: Type restrictions have not yet been implemented in system
Copy link
Contributor

Choose a reason for hiding this comment

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

TODOs should link to a bug. If there isn't one, file one first, then link to the bug like TODO(https://github.com/dart-lang/native/issues/...): Type restrictions...

Same below

(previous, element) =>
previous.union(dependencyVisitor.visitDeclaration(element)));
final depDecls =
(allDecls ?? decls).where((d) => deps.contains(d.name));
Copy link
Contributor

Choose a reason for hiding this comment

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

Names aren't unique (eg two nested types, nested in different parent types, can have the same name). Use the declaration id instead.

Better yet, why not just have the dependency visitor construct a Set<Declaration> directly, rather than constructing a Set<String> and then looking up the declaration from the string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the types do not reference declarations directly (like function parameters, variable types), which therefore means that declarations need to be looked up eventually.

The lookup is done afterwards to prevent multiple lookups and just have a single lookup per visitation.

Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't ever need to look up types by name or id, after parsing is complete. The AST already has references to the declarations. Check if the ReferredType is a DeclaredType.


// since we are making such visitations on normal declarations in a file,
// we do not need to filter out primitives at the moment
cont.addAll(_getTypeNames(type.swiftType));
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't have to re-parse these types from strings. We did all the parsing already, and now have a nice clean AST to work with. In this case you can just check if the type is a DeclaredType, and get its id or declaration or whatever. Also need to handle the OptionalType case.

@coveralls
Copy link

Coverage Status

coverage: 88.691% (-0.09%) from 88.784%
when pulling 94926ae on nikeokoronkwo:swift2objc-filters
into d79f68c on dart-lang:main.

@liamappelbe liamappelbe merged commit f2a0e28 into dart-lang:main Dec 16, 2024
6 checks passed
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