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

preserve do syntax in AST #23718

Merged
merged 1 commit into from
Dec 20, 2017
Merged

preserve do syntax in AST #23718

merged 1 commit into from
Dec 20, 2017

Conversation

JeffBezanson
Copy link
Sponsor Member

part of #21774

@JeffBezanson JeffBezanson added breaking This change will break code parser Language parsing and surface syntax labels Sep 14, 2017
@ararslan
Copy link
Member

Can you remind me on why triage was in favor of preserving do? Thinking about it now, this seems like an annoying thing to have to remember to special-case when writing a macro that handles anonymous functions like this.

@JeffBezanson
Copy link
Sponsor Member Author

I'm not 100% convinced myself, but I can think of some reasons:

  • This allows better pretty-printing of expressions.
  • Converting do to its lowered form re-orders expressions, which is a bit unintuitive.
  • Keno felt that the do form might be useful for macro DSLs to intercept.

@JeffBezanson JeffBezanson added the needs decision A decision on this change is needed label Sep 20, 2017
@MikeInnes
Copy link
Member

the do form might be useful for macro DSLs to intercept.

Yes, this is generally a good reason to preserve this kind of information. If you don't care it might be an extra concern (though not if you're just walking over all lambdas).

The extra work for do blocks pales in comparison with things like function definitions, which we don't normalise at all. So we need a more general solution there anyway, probably involving exposing some lowering tools in Base.Meta.

@JeffBezanson JeffBezanson added this to the 1.0 milestone Nov 20, 2017
@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Nov 20, 2017
@JeffBezanson
Copy link
Sponsor Member Author

1.0 decision needed.

@MikeInnes
Copy link
Member

MikeInnes commented Dec 7, 2017

So right now I'm generating expressions that look like this:

map(((quetzal,)->(Dagger.Thunk)((penguin,)->begin  
        bear = (Array{Any,1})(quetzal)
        for jellyfish = quetzal
            bear[jellyfish] = penguin[jellyfish] + 1
        end
        bear
    end
end, chslice(cat, domain(cat, 1)))), domprod(domain(cat, 1)))

With this change the code would output like this:

map(domprod(domain(cat, 1))) do quetzal
    (Dagger.Thunk)(chslice(cat, domain(cat, 1)))) do penguin 
        bear = (Array{Any,1})(quetzal)
        for jellyfish = quetzal
            bear[jellyfish] = penguin[jellyfish] + 1
        end
        bear
    end
end

Which is a significant readability improvement.

@StefanKarpinski
Copy link
Sponsor Member

We don't really need this change to get that, we just need some decent heuristics on when to use do syntax printing a function call. Just off the top of my head: the function is the first argument and the body of the function definition is multiline.

@StefanKarpinski
Copy link
Sponsor Member

Triage is ambivalent about this. @JeffBezanson, it's your call. Do what your heart tells you.

@StefanKarpinski
Copy link
Sponsor Member

Triage: let's do it. Merge as soon as CI passes.

@ararslan ararslan removed the triage This should be discussed on a triage call label Dec 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code needs decision A decision on this change is needed parser Language parsing and surface syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants