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

Support anonymous function in splitdef/combinedef #140

Merged
merged 1 commit into from
May 13, 2020

Conversation

tkf
Copy link
Contributor

@tkf tkf commented Apr 30, 2020

Copy link
Collaborator

@cstjean cstjean left a comment

Choose a reason for hiding this comment

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

LGTM

I'm not super happy with the exponentially increasing number of branches, though. I wonder if we shouldn't rewrite it as

all_args = kwargs === nothing ? args : Expr(:parameters, kwargs...)
fcall = func === nothing ? args : Expr(:call, func, all_args...)
fcall_with_rtype = rtype === nothing ? fcall : :($fcall::$rtype)
fcall_with_rtype_and_where = ...
etc.

It could be done in another PR. It looks like it could be a lot shorter.

@tkf
Copy link
Contributor Author

tkf commented Apr 30, 2020

if we shouldn't rewrite it as

It sounds like a great idea. I just tried to stick with the existing pattern.

@MasonProtter
Copy link

Perhaps MacroTools.jl should just use https://github.com/invenia/ExprTools.jl for splitdef and combinedef? ExprTools.jl's implementation is a bit more robust and performant from what I understand.

@cstjean
Copy link
Collaborator

cstjean commented May 13, 2020

That's #132

@MasonProtter
Copy link

🤦 oops, I should read more carefully.

@cstjean cstjean merged commit 75535ac into FluxML:master May 13, 2020
@tkf tkf deleted the anonymous-splitdef branch May 13, 2020 19:21
@devmotion
Copy link
Contributor

Is it possible to tag a release with these changes? I'd like to use this functionality and since I'm already using MacroTools it would be nice to avoid having to add ExprTools (which handles anonymous functions fine) as an additional dependency.

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.

Add splitdef for lambda functions
4 participants