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

Extend support for :asvalue() + test #209

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Extend support for :asvalue() + test #209

wants to merge 1 commit into from

Conversation

nicolasvasilache
Copy link
Contributor

Added some more cases for asvalue() and a unit test that I find useful in what I am doing (line 871 ..).
About all the line changes, my emacs config automatically removes trailing whitespaces; let me know if this is too annoying and I'll submit another PR without those.

@aiverson
Copy link
Contributor

Constant folding in :asvalue looks very useful.

Does this handle operator overloading on custom types?

Can we integrate this change? Can we fix the whitespace on everything in master?

@elliottslaughter
Copy link
Member

It would definitely be easier to merge without the whitespace changes. If @nicolasvasilache or someone else has time to do that it would be great, otherwise I may get to it eventually (but don't wait on me, eventually might be a long time).

@aiverson
Copy link
Contributor

I've found a few bad cases that should be mentioned related to this. For one, (`nil):asvalue() is difficult to tell apart from a failed constant resolution. Also, it currently doesn't resolve things like (quote in 5 end):asvalue()

I think adding an isvalue/isconstant method and an istype method to check for a quote being a constant value or a type would relieve the difficulty of checking for errors and remove the ambiguity/failure case where the value of the quote may be nil.

@aiverson
Copy link
Contributor

I'd like to request that the constant folding additions included here also be applied to the isconstant function. Or some way to test whether something is a constant or not, and get the value. Currently, quote:asvalue will return a symbol when given a quote of looking up the value at a variable, which doesn't fit the usecase of extracting a constant value from a quote.

@aiverson
Copy link
Contributor

As a corollary to my last comment, I'd like to propose that any operation needed by the standard library that needs any awareness of the internal representation of trees be made part of the public API of terralib with respect to quotes so that refactors affecting the AST like the addition of new node kinds or anything can remain more interface compatible with less breakage.

I'm not sure that this is entirely possible to do, but it seems like a good goal.

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.

3 participants