Skip to content
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

[swift2objc] Visitor infra #1834

Merged
merged 5 commits into from
Jan 14, 2025
Merged

[swift2objc] Visitor infra #1834

merged 5 commits into from
Jan 14, 2025

Conversation

liamappelbe
Copy link
Contributor

This is the same infra as ffigen uses: #1638.

  • Copied the AstNode classes etc from ffigen
  • Made all the AST classes extends AstNode, and implemented the visit and visitChildren methods.
    • This meant that BuiltInDeclaration had to be converted from an enum to a class
  • Rewrote DependencyVisitor using the Visitation class

Ideally we'd share the AstNode, Visitor, and Visitation classes, but we don't really have a way of sharing internal code between native packages yet.

Part of #1358

@liamappelbe liamappelbe requested a review from dcharkes December 19, 2024 01:00
@coveralls
Copy link

coveralls commented Dec 19, 2024

Coverage Status

coverage: 87.4% (-0.3%) from 87.691%
when pulling 29cd1bc on swiftvisit2
into 3832e4b on main.

@liamappelbe liamappelbe enabled auto-merge (squash) December 20, 2024 03:27
@liamappelbe liamappelbe disabled auto-merge January 2, 2025 03:25
@liamappelbe
Copy link
Contributor Author

@dcharkes @HosseinYousefi This is ready to go. PTAL


final _logger = Logger('swift2objc.visitor');

/// Wrapper around [Visitation] to be used by callers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The documentation could be improved.

If I understand it correctly, the visitor does the traversal, and the visitation contains the methods on what to modify for each node type.

"Wrapper" doesn't say what it's responsibility is and isn't.

I'd maybe call the the class that determines the traversal order the Traverser. Or TraversalStrategy (terminology taken from https://strategoxt.org/, which also explicitly separates the function applied to a node on visit, and the traversal strategy of traversing the whole tree).

Copy link
Contributor Author

@liamappelbe liamappelbe Jan 14, 2025

Choose a reason for hiding this comment

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

Done. I prefer to have the names of tightly coupled classes like this be closely related though. Visitor and Visitation makes them sound more related than Traverser and Visitation.

/// ## Creating a new type of [AstNode]
///
/// For example, adding a class `Foo` which extends `Bar`, which somewhere up
/// the heirarchy extends [AstNode]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// the heirarchy extends [AstNode]:
/// the hierarchy extends [AstNode]:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// distinction is why [Visitor] and [Visitation] are seperate classes.
///
/// The `visitFoo` methods in this class should reflect the inheritance
/// heirarchy of all AstNodes. Eg `visitChild` should default to calling
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// heirarchy of all AstNodes. Eg `visitChild` should default to calling
/// hierarchy of all AstNodes. Eg `visitChild` should default to calling

:P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

visitEnumDeclaration(node);

/// Default behavior for all visit methods.
void visitAstNode(AstNode node) => node..visitChildren(visitor);
Copy link
Member

Choose a reason for hiding this comment

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

nit: Could also be node.visitChildren instead of ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. A leftover from when this was a transformer that returned nodes.

Copy link

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
Changelog Entry ✔️
Package Changed Files

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

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 symbols
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

@liamappelbe liamappelbe merged commit bc1ca8c into main Jan 14, 2025
15 checks passed
@liamappelbe liamappelbe deleted the swiftvisit2 branch January 14, 2025 04:54
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.

4 participants