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

Some expression might be evaluated several times #11

Open
clouds56 opened this issue Nov 9, 2018 · 4 comments
Open

Some expression might be evaluated several times #11

clouds56 opened this issue Nov 9, 2018 · 4 comments

Comments

@clouds56
Copy link

clouds56 commented Nov 9, 2018

Now macroexpand(Main, :(@pipe a|>f|>g(_,_,b))) would be expanded as :(g(f(a), f(a), b)).

I think it should be more appreciated to be expanded as something like

:((a |> f) |> (x->begin
              #= REPL[23]:1 =#
              g(x, x, b)
          end))
@oxinabox
Copy link
Owner

oxinabox commented Nov 9, 2018

Correct, it will be expanded multiple times, as per the readme.

Expanding it into a function call like you suggest would solve the problem,
but it would also change what the package does slightly.
Since now it is introducing an anon function,
which it then hopes will be optimised away.
Which is potentially slower in the case of single inputs.

In general I find that piping like this mostly only makes sense for single inputs.
(It certainly does not make sense for multiple outputs, except when you can pipe through first and last)

This package is kinda in end-of-life,
since this kind of functionality is going to become part of the language in 1.2 or 1.3.

I might be willing to entertain such a PR, if it came with benchmarks showing no performance regressions,
though it is a breaking change (in theory) since the call the function multiple times is documented behavior.

Alternatively, consider some of the alternative packages
JuliaLang/julia#5571 (comment)

@racinmat
Copy link
Contributor

I found an another case of this bug: macroexpand(Main, :(@pipe a |> b(_) .|> c(_) |> d)), which is expanded to d((a |> b(a)) .|> (var"##516"->begin c(var"##516") end)). I'll try to make a PR with performance tests if I'll have time.

@racinmat
Copy link
Contributor

racinmat commented Aug 2, 2020

Should the benchmark contain only the macro expansion, or also the optimization?
I assume we want to benchmark also the optimization, which does not happen at macro expansion, right?

@oxinabox
Copy link
Owner

I think more end to end.
which some vaguely realistic use cases.
We want to see if normal code might run into the closure boxing issue for example.

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