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

Divide the run() function in smaller portions #366

Closed
32 tasks done
Razican opened this issue May 2, 2020 · 6 comments
Closed
32 tasks done

Divide the run() function in smaller portions #366

Razican opened this issue May 2, 2020 · 6 comments
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed technical debt
Milestone

Comments

@Razican
Copy link
Member

Razican commented May 2, 2020

Currently, the run() function is around 500 lines long. This is going to quickly become unmaintainable, so we need to divide this.

The first thing that comes to my mind to improve this is to just create new function for each match arm, but as we saw in the parser, we can probably do this better and easier to understand.

One option is to have an Executable trait, as I proposed in #304. This trait would just be as simple as this:

trait Executable {
	fn run<E>(interpreter: &mut E) -> ResultValue where E: Executor;
}

Then, I think that we should change the Node enum to be just a Box<dyn Executable>. And each Node type should be a struct where we could use Box to box dynamic types, or we could use compile-time generics to have less allocations (but longer compile times).

Then we implement this trait individually for each of the node types, which is easily extensible, and we reduce the run() function to:

fn run(&mut self, node: &Node) -> ResultValue {
	node.run(&mut self)
}

Tracking:

  • Node::ArrayDecl
  • Node::ArrowFunctionDecl
  • Node::Assign
  • Node::BinOp
  • Node::Block
  • Node::Break
  • Node::Call
  • Node::ConditionalOp
  • Node::Const
  • Node::ConstDeclList
  • Node::Continue
  • Node::DoWhileLoop
  • Node::FunctionDecl
  • Node::FunctionExpr
  • Node::GetConstField
  • Node::GetField
  • Node::ForLoop
  • Node::If
  • Node::LetDeclList
  • Node::Identifier
  • Node::New
  • Node::Object
  • Node::Return
  • Node::Switch
  • Node::Spread
  • StatementList
  • Node::Throw
  • Node::Try
  • Node::This
  • Node::UnaryOp
  • Node::VarDeclList
  • Node::WhileLoop
@Razican Razican added help wanted Extra attention is needed technical debt labels May 2, 2020
@HalidOdat
Copy link
Member

Hmmm. I think doing this at compile time is better than having to do it at runtime.
Using Box<dyn Trait> has to allocate and use dynamic dispatch (vtable) which would worsen the execution time.

@Razican
Copy link
Member Author

Razican commented May 2, 2020

Hmmm. I think doing this at compile time is better than having to do it at runtime.
Using Box<dyn Trait> has to allocate and use dynamic dispatch (vtable) which would worsen the execution time.

So, Node would be Node<T: Executable>, but that would make it more difficult to have for example a Vec<Node> of different kinds, and we already use Box inside of nodes, this would just make a Box and only one allocation per node.

@Razican
Copy link
Member Author

Razican commented May 7, 2020

A different option would be to still have an enum, but each of the enum members would be just a box to a given Executable.

@Razican Razican added this to the v0.9.0 milestone May 14, 2020
@Razican Razican linked a pull request May 14, 2020 that will close this issue
32 tasks
@Razican Razican self-assigned this May 14, 2020
@Razican Razican removed a link to a pull request May 23, 2020
32 tasks
@Razican Razican removed their assignment May 26, 2020
@Razican Razican added the good first issue Good for newcomers label May 26, 2020
@Razican
Copy link
Member Author

Razican commented May 26, 2020

Tracking list of nodes added, now that we have the infrastructure in place.

This could be a very good first issue, since it shows you how the AST nodes work, so feel free to claim it and to ask for help!

@Lan2u
Copy link

Lan2u commented May 26, 2020

I shall give it a go :)

@Razican
Copy link
Member Author

Razican commented May 26, 2020

I shall give it a go :)

Go ahead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed technical debt
Projects
None yet
Development

No branches or pull requests

3 participants