Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Redesign Proposal #22
Redesign Proposal #22
Changes from 8 commits
5f0b5d9
2052ebd
3cb7568
f235686
bc1ec14
bcc4574
e2aef1a
47610e7
85d0931
62ff72a
59da1f7
eef8273
a3eacea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again should not be in this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I believe this one should stay in this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This package is used to define an AST interface so that pattern matchers can match against the AST and traverse it. I think it's a little bit of a mistake to introduce
@rule
with the example of+(~a, ~b)
, because in many IRs we would want to write this as:call(+, ~a, ~b)
. Not all ASTs will have something analogous to a function call, and it's unclear what benefit would be derived from standardizing this notion here.operation
andarguments
should live in packages that define ASTs that can give these functions meaning.TL;DR: I don't think we can define what the new meaning of
operation
is supposed to do in a way that captures all of its possible use cases in downstream packages, so I think it should not be in this package.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess most of what the (old/current) dependents of this package do:
AbstractPat
AST for patternsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use trait like abstractrees? Will make a larger comment below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shashi, do we agree now that
function operation end
is fine as-is, just adjust the documentation to say that this one is optional?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x
is no longer defined here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, though I wonder if AbstractTrees could help here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach to building ASTs results in huge slowdowns in compilers, because compilers specialize on each combination of node types. I appreciate the idea of adding an optional AST easy implementation feature, could we add it in a separate PR?
A few ideas:
head_symbol
is not part of the TermInterface interface, it's just part of the easy-mode ast builder.If we feel like an optimized AST implementation is out of scope, we shouldn't include programming patterns that will inconvenience users later on with performance issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file seems like the right approach to me
This file was deleted.