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.

Sign up for GitHub

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

Jump to bottom

Creating a Simple AST and an Excluder Visitor #1710

Open
marshelino-maged opened this issue Nov 9, 2024 · 3 comments · May be fixed by #1755
Open

Creating a Simple AST and an Excluder Visitor #1710

marshelino-maged opened this issue Nov 9, 2024 · 3 comments · May be fixed by #1755
Labels
package:jni package:jnigen

Comments

@marshelino-maged
Copy link

Started with making the Visitor interface ... as dart doesn't support overloading methods with different parameters, I used generic types, and the Element interface that will accept this visitor.

abstract class Visitor<T, R> {
  R visit(T node);
}

abstract class Element<T> {
  R accept<R>(Visitor<T, R> visitor);
}

then the simple AST only containing (classes, methods)

class Class implements Element<Class> {
  Class(this.name, this.methods):isExcluded=false;
  String name;
  List<Method> methods;
  // removed fields for simplicity
  // List<Field> fields;
  bool isExcluded;

  @override
  R accept<R>(Visitor<Class, R> visitor) {
    return visitor.visit(this);
  }
}

class Method implements Element<Method> {
  Method(this.name, this.isPrivate, this.isAbstract):isExcluded=false;
  String name;
  bool isPrivate;
  bool isAbstract;
  bool isExcluded;

  @override
  R accept<R>(Visitor<Method, R> visitor) {
    return visitor.visit(this);
  }
}

then implementing the Excluder Visitors

class ExcludeClass implements Visitor<Class, void> {

  @override
  void visit(Class c) {
    if(c.name.startsWith("_")) {
      c.isExcluded = true;
    }
  }
}

class ExcludeMethod implements Visitor<Method, void> {

  @override
  void visit(Method method) {
    if(method.isAbstract) {
      method.isExcluded = true;
    }
  }
}
@HosseinYousefi
Copy link
Member

Thanks for taking this up! We probably want to have them all in one class. It's nicer if we have a single visitor class, with multiple void methods: visitClass, visitMethod, ... So the user doesn't have to think about creating multiple classes.

@marshelino-maged
Copy link
Author

Thank you for the note! Combining everything into a single visitor class makes the design cleaner and easier to use. Here’s the revised code based on your suggestion:

abstract class Visitor {
  void visitClass(Class c);
  void visitMethod(Method method);
}

abstract class Element {
  void accept(Visitor visitor);
}
class ExcludeVisitor implements Visitor {

  @override
  void visitClass(Class c) {
    if(c.name.startsWith("_")) {
      c.isExcluded = true;
    }
  }

  @override
  void visitMethod(Method method) {
    if(method.isAbstract) {
      method.isExcluded = true;
    }
  }
}

In the Class element, I've updated the accept method to automatically propagate the visitor to each method in its methods list, making it easier for the user to apply the visitor across all contained methods:

// simple AST
class Class extends Element {
  Class(this.name, this.methods):isExcluded=false;
  String name;
  List<Method> methods;
  bool isExcluded;

  @override
  accept(Visitor visitor) {
    for (var method in methods) {
      method.accept(visitor);
    }
    visitor.visitClass(this);
  }
}

class Method extends Element {
  Method(this.name, this.isPrivate, this.isAbstract):isExcluded=false;
  String name;
  bool isPrivate;
  bool isAbstract;
  bool isExcluded;

  @override
  accept(Visitor visitor) {
    visitor.visitMethod(this);
  }
}

@HosseinYousefi
Copy link
Member

Minor adjustment is to visit the class first, and not visit the methods if the class is already excluded by the visitor. But you can go ahead and open a PR for this.

@marshelino-maged marshelino-maged linked a pull request Nov 26, 2024 that will close this issue
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
package:jni package:jnigen
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[jnigen] Simple AST implementation marshelino-maged/native
2 participants
@HosseinYousefi @marshelino-maged