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

#110 doesn't rewrite properly #111

Closed
sbromberger opened this issue Jul 2, 2015 · 4 comments
Closed

#110 doesn't rewrite properly #111

sbromberger opened this issue Jul 2, 2015 · 4 comments

Comments

@sbromberger
Copy link
Contributor

Consider:

julia> macroexpand(:(TarjanVisitor(n::Int) = TarjanVisitor(
                         @compat Vector{Int}(),
                         @compat Vector{Int}(),
                         zeros(Int, n),
                         @compat Vector{Vector{Int}}()
              )))
:(TarjanVisitor(n::Int) = begin  # none, line 1:
            TarjanVisitor((Vector{Int}(),(Vector{Int}(),zeros(Int,n),Vector{Vector{Int}}())))
        end)

julia> macroexpand(:(@compat TarjanVisitor(n::Int) = TarjanVisitor(
                         Vector{Int}(),
                         Vector{Int}(),
                         zeros(Int, n),
                         Vector{Vector{Int}}()
                     )))
:(TarjanVisitor(n::Int) = begin  # none, line 1:
            TarjanVisitor(Vector{Int}(),Vector{Int}(),zeros(Int,n),Vector{Vector{Int}}())
        end)

When @compat is placed BEFORE the beginning of the constructor, things are fine. When @compat is used WITHIN the constructor, things fail (see https://travis-ci.org/JuliaGraphs/LightGraphs.jl/builds/69349027 for an example).

@sbromberger
Copy link
Contributor Author

A simpler case that shows what appears to be incorrect nesting of parens (second example):

julia> macroexpand(:(@compat Foo(n::Int) = Vector{Int}(), Vector{Int}(), Int))
:(Foo(n::Int) = begin  # none, line 1:
            (Vector{Int}(),Vector{Int}(),Int)
        end)

julia> macroexpand(:(Foo(n::Int) = @compat Vector{Int}(), @compat Vector{Int}(), Int))
:(Foo(n::Int) = begin  # none, line 1:
            (Vector{Int}(),(Vector{Int}(),Int))
        end)

@simonster
Copy link
Member

This is just the way macros are. @macro x, y parses as @macro((x, y)) and not @macro(x),y. This has tripped people up before (e.g. #71), but there's nothing we can do about it in Compat, since we can't actually tell the difference between the first two expressions above. It's arguable that @macro x, y is ambiguous and should be a syntax error, but that change would have to happen in Julia itself.

@sbromberger
Copy link
Contributor Author

Thank you - this makes sense. The solution, I guess, is to use explicit parentheses with compat.

@kmsquire
Copy link
Member

kmsquire commented Jul 6, 2015

@compat does recurse into expressions (as you found by putting it out front of a function). Of course, putting it around a large chunk of code would probably be slow (but should work), so you should pick where you put it judiciously. But in front of a function call or definition, even, should generally be okay.

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

No branches or pull requests

3 participants