-
Notifications
You must be signed in to change notification settings - Fork 7
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
Implement AST reflection #213
Conversation
@tedinski @ericvanwyk Anything that I should fix before merging this? |
runtime/java/src/common/Node.java
Outdated
public abstract String[] getAnnoNames(); | ||
|
||
/** | ||
* Used for debugging, stack traces especially. |
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.
Text from copy and paste?
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.
Oops...
General thoughts:
Probably good to merge. |
Ok with me.
…On Thu, Jan 11, 2018 at 3:45 PM, Ted Kaminski ***@***.***> wrote:
General thoughts:
1. Not sure if we should have AST have a pp, but for now okay no
problem. But I think we could probably avoid the dependency on langutil in
the final version?
2. In the end, we probably need tests that do better than compare to
string (too brittle?), but obviously I think that's hard to do yet? So ok.
Probably good to merge.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#213 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADOhd9z8ScxIzeip8BYPQW-4nY2CMPz1ks5tJoD2gaJpZM4RTxKM>
.
|
Yeah, I suppose those equations could move to aspects in the langutil library.
Agreed, we really need a derivable equality typeclass for this... |
This implements the
reflectAST
operation as described in #212. Next up is reify.