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

Don't rely on reduce being fold-left #3

Closed
stevengj opened this issue Aug 13, 2013 · 3 comments
Closed

Don't rely on reduce being fold-left #3

stevengj opened this issue Aug 13, 2013 · 3 comments
Assignees
Milestone

Comments

@stevengj
Copy link
Contributor

As you mentioned today, the code

mdo_desugar(exprIn) = reduce(mdo_desugar_helper, :(), reverse(exprIn.args))

assumes that the associativity of reduce is fold-left, which is true of the current implementation but is undocumented, and may change (JuliaLang/julia#4046).

In any case,

function mdo_desugar(exprIn)
    r = :()
    for i = length(exprIn.args):-1:1
        r = mdo_desugar_helper(exprIn.args[i], r)
    end
    r
end

has the additional virtue of being faster (since it avoids the call to reverse and the allocation of a new array), not that it probably matters to you.

@pao
Copy link
Collaborator

pao commented Aug 13, 2013

virtue of being faster

This is so clearly not relevant to any of this code :)

I think I used to have it written something like that at one point during its evolution, but the higher-order-function approach was appealing for reasons that are completely impractical, but that fits well with the context. But in light of JuliaLang/julia#4046 I agree that it'll need to be changed. I'll roll it in when I update all my packages around 0.2-rc1 time.

@jfarid27
Copy link
Collaborator

jfarid27 commented Oct 4, 2017

Closing since it's old

@jfarid27 jfarid27 closed this as completed Oct 4, 2017
@stevengj
Copy link
Contributor Author

stevengj commented Oct 5, 2017

The bug in mdo_desugar (relying on associativity of reverse) is still there in the current code. It should just call foldl.

stevengj added a commit to stevengj/Monads.jl that referenced this issue Oct 5, 2017
@jfarid27 jfarid27 reopened this Oct 5, 2017
@jfarid27 jfarid27 added this to the v0.0.1 milestone Oct 5, 2017
ulysses4ever pushed a commit that referenced this issue Oct 5, 2017
don't rely on reduce being fold-left (fix #3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants