Skip to content

Conversation

@navidaminnn
Copy link
Collaborator

current state is that it's working with most test cases, but struggles with more complex ones.

@fhackett
Copy link
Collaborator

fhackett commented Feb 19, 2025

Ideas from the meeting about this:

  • use structure below for Expr, so the parsing pattern doesn't match both input and output
  • try to have one token per operator, and do "low precedence" or "high precedence" categories using lists, sets, etc
  • I would have the simplify pass be in a different pass sequence, so you can (1) check that you get the right tree out of the parse, and (2) check that you can evaluate ASTs, key being that you can do these separately
  • extra DCal feature you can use: Node.Embed. If you want the Number expressions to contain actual Int objects, you can do that! There's a grammar for it too: (very basic example) https://github.com/DistCompiler/dcal/blob/main/Wellformed.test.scala#L28

(for point 1)

(Expr
  (Plus
    (x)
    (y)))

where

Expr ::= choice(
  Plus,
  Minus,
  Times,
  Divide,
)

private val mulDivPass = passDef:
wellformed := inputWellformed.makeDerived:
Node.Top ::=! repeated(choice(Number, Expression, AddOp, SubOp))

Copy link
Collaborator

Choose a reason for hiding this comment

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

idea: wrap Number in separate Expression in separate pre-process pass here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

opted to wrap Number in an Expression node in the CalcReader

@navidaminnn
Copy link
Collaborator Author

Ideas from the meeting about this:

  • use structure below for Expr, so the parsing pattern doesn't match both input and output
  • try to have one token per operator, and do "low precedence" or "high precedence" categories using lists, sets, etc
  • I would have the simplify pass be in a different pass sequence, so you can (1) check that you get the right tree out of the parse, and (2) check that you can evaluate ASTs, key being that you can do these separately
  • extra DCal feature you can use: Node.Embed. If you want the Number expressions to contain actual Int objects, you can do that! There's a grammar for it too: (very basic example) https://github.com/DistCompiler/dcal/blob/main/Wellformed.test.scala#L28

(for point 1)

(Expr
  (Plus
    (x)
    (y)))

where

Expr ::= choice(
  Plus,
  Minus,
  Times,
  Divide,
)

From this list of ideas, the only one remaining is to leverage Node.Embed to work directly with Int objects. I'm currently working on adding this.

@fhackett
Copy link
Collaborator

The Node.Embed could be another PR probably, this is already fine code-wise.

Given a README that directs a new person how they should read this, etc, this is all done I think.

@navidaminnn
Copy link
Collaborator Author

Added the README, primarily focusing on what each file does and how the passes and wellformed definitions work. I also included images that outline how the AST transforms for a simple visualization.

cc: @fhackett - currently feels like some of the explanations for the different components are a bit unclear, LMK what you think.

@fhackett
Copy link
Collaborator

fhackett commented Apr 9, 2025

@navidaminnn I agree that the descriptions could grow a fair amount. They would ideally include even more specific step by step notes on how to write different elements, including Wellformed, specific pattern composition, etc.

That said, I think good as a starting point, and all the key points are there. This is so much better than literally nothing that I'd prefer to merge this now and then work on it more later.

@fhackett
Copy link
Collaborator

fhackett commented Apr 9, 2025

Ok, so looking into the random CI failure, it seems to have nothing to do with your changes. The longest running test just barely times out its 30s deadline, and I can fix that best in main anyway.

Merging.

@fhackett fhackett marked this pull request as ready for review April 9, 2025 22:01
@fhackett fhackett merged commit a718fff into DistCompiler:main Apr 9, 2025
6 of 14 checks passed
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.

2 participants