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

IfNode rewrite #62

Closed
wants to merge 1 commit into from
Closed

IfNode rewrite #62

wants to merge 1 commit into from

Conversation

rfdrake
Copy link

@rfdrake rfdrake commented Feb 27, 2013

This is probably messed up because I don't understand Scala. By the time I developed an idea of what I was doing I thought some things might not be needed anymore, and may have gotten nasty in a few places (using null to avoid type mismatches between UndefLiteral and IfStruct)

In general, the theory I was trying to get at was that IfNode() should be somewhat recursive. The Parser should build up the IfNode structure and send it. If this were C I would say that else_node is a pointer to the next IfNode if it's defined (in this case, since it's an object it might be getting instantiated as the IfStruct is built and thus consume memory when it will never be executed. We don't want that, but I don't know how to tell Scala about pointers)

Comments welcome. This can certainly be trashed if it does a bad thing. :)

@stevan
Copy link
Member

stevan commented Feb 27, 2013

Robert,

I like the design, especially where it is recursive, I think that works quite well. I have really only one issue, which is that I would prefer to not use null, but instead to use the Option type. This would make the IfStruct look like this, then you would just need to change it in the interpreter when you are checking the else branch.

case class IfStruct (
  var condition: AST,
  var body: AST, 
  var else_node: Option[IfStruct] = None
) extends AST

if you could make this change, I would be happy to accept this.

Thanks,

  • Stevan

(p.s. - for some reason github is saying that the pull request cannot be automatically merged, you might need to update the branch you are working on to fix that)

@rfdrake
Copy link
Author

rfdrake commented Mar 1, 2013

Closing this to reopen under a new branch.

@rfdrake rfdrake closed this Mar 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants