Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

function next(eit::EdgeIter, state) modifying state #419

Closed
IssamT opened this issue Aug 27, 2016 · 11 comments
Closed

function next(eit::EdgeIter, state) modifying state #419

IssamT opened this issue Aug 27, 2016 · 11 comments

Comments

@IssamT
Copy link
Contributor

IssamT commented Aug 27, 2016

I think state should not be modified by the function next.

http://stackoverflow.com/questions/39181313/julia-iteration-start-next-done-side-effects

Fixing it requires a minor modification. I can suggest a PR if you don't mind.

@sbromberger
Copy link
Owner

Please do. Thanks!

@jpfairbanks
Copy link
Contributor

Is the only argument against mutating the state variable is to allow caching the iteration state? Is there an example of code doing this? I can only recall seeing the start, next, done interface used in for loops, which do not cache the state variable.

@sbromberger
Copy link
Owner

@Keno - your input appreciated here, especially given http://stackoverflow.com/questions/39181313/julia-iteration-start-next-done-side-effects - see also the PR (#423).

@Keno
Copy link

Keno commented Aug 29, 2016

Is the only argument against mutating the state variable is to allow caching the iteration state? Is there an example of code doing this? I can only recall seeing the start, next, done interface used in for loops, which do not cache the state variable.

This capability is used extensively in some of the iterator wrappers e.g. those from https://github.com/JuliaLang/Iterators.jl. In general the state variable is not very large and immutable. If that's not the case, the trade-off needs to be carefully considered.

@Keno
Copy link

Keno commented Aug 29, 2016

In this case, I haven't looked at the code too much, but from that PR, it looks like EdgeIterState could be made immutable in which case the non-mutating version is probably better.

@sbromberger
Copy link
Owner

the PR shows O(n) allocations with immutable state. This looks like it would be a big problem. Your thoughts?

@Keno
Copy link

Keno commented Aug 29, 2016

Did you change EdgeIterState to an immutable?

@jpfairbanks
Copy link
Contributor

PR #423 does not. In fact it creates a new instance and then mutates it.

@IssamT
Copy link
Contributor Author

IssamT commented Aug 29, 2016

I am sorry I just see the conversation here and only read the messages in #423. I will try to use the immutable as @Keno suggests

@IssamT
Copy link
Contributor Author

IssamT commented Aug 29, 2016

I m surprised by the results when using immutable. To be honest I don't understand the reason behind that... I added the results in #423

@sbromberger
Copy link
Owner

sbromberger commented Aug 29, 2016

Closing via #423 - thanks for the really great discussion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants