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

Fix #7 #25

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix #7 #25

wants to merge 2 commits into from

Conversation

melonedo
Copy link

I am not sure whether extra care should be taken when we allow Expr(:quot) to be recursed into, but this fixes the issue.

Copy link
Owner

@c42f c42f left a comment

Choose a reason for hiding this comment

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

Thanks!

Yes this does require extra care — you need to track whether you're inside a quoted section of the AST or inside an interpolation (inside a $) as part of the recursive expansion. Otherwise the following incorrect result will occur

julia> @_ map(:(_ + 1), 1:4)
4-element Vector{Expr}:
 :(_1 + 1)
 :(_1 + 1)
 :(_1 + 1)
 :(_1 + 1)

The additional tricky aspect is that this may be arbitrarily nested. For example, we should have:

julia> @_ map(:($(_ + :(_+2)) + _), 1:4)
4-element Vector{Expr}:
 :((1 + :(_ + 2)) + _)
 :((2 + :(_ + 2)) + _)
 :((3 + :(_ + 2)) + _)
 :((4 + :(_ + 2)) + _)

@@ -102,9 +102,7 @@ const _pipeline_ops = [:|>, :<|, :∘, :.|>, :.<|]

function lower_underscores(ex)
if ex isa Expr
if isquoted(ex)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this should be removed — it currently ignores Expr(:meta, ...) which it should continue doing (and Expr(:inert) I guess... though :meta and particularly :inert are quite obscure!)

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