Yeah... but we still have to fix the iterator.
On Sun, Apr 14, 2013 at 8:58 AM, Robin Anil <[email protected]> wrote: > Here is an iteration style that works as is with today's behaviour of > hasNext > > 1. > 2. Element thisElement = null; > 3. Element thatElement = null; > 4. boolean advanceThis = true; > 5. boolean advanceThat = true; > 6. > 7. Iterator<Element> thisNonZero = this.iterateNonZero(); > 8. Iterator<Element> thatNonZero = x.iterateNonZero(); > 9. > 10. double result = 0.0; > 11. while (true) { > 12. *if (advanceThis) { > * > 13. * if (!thisNonZero.hasNext()) { > * > 14. * break; > * > 15. * } > * > 16. * thisElement = thisNonZero.next(); > * > 17. * } > * > 18. * if (advanceThat) { > * > 19. * if (!thatNonZero.hasNext()) { > * > 20. * break; > * > 21. * } > * > 22. * thatElement = thatNonZero.next(); > * > 23. * }* > 24. if (thisElement.index() == thatElement.index()) { > 25. > 26. result += thisElement.get() * thatElement.get(); > 27. advanceThis = true; > 28. advanceThat = true; > 29. } else if (thisElement.index() < thatElement.index()) { > 30. advanceThis = true; > 31. advanceThat = false; > 32. } else { > 33. advanceThis = false; > 34. advanceThat = true; > 35. } > 36. } > > > On Sat, Apr 13, 2013 at 1:47 AM, Ted Dunning <[email protected]> > wrote: > > > The caller is not at fault here. The problem is that hasNext is > advancing > > the iterator due to a side effect. The side effect is impossible to > avoid > > at the level of the caller. > > > > Sent from my iPhone > > > > On Apr 12, 2013, at 12:22, Sean Owen <[email protected]> wrote: > > > > > I'm sure I did (at least much of) the AbstractIterator change so blame > > > me... but I think the pattern itself is just fine. It's used in many > > > places in the project. Reusing the value object is a big win in some > > > places. Allocating objects is fast but a trillion of them still adds > > > up. > > > > > > It does contain a requirement, and that is that the caller is supposed > > > to copy/clone the value if it will be used at all after the next > > > iterator operation. That's the 0th option, to just fix the caller > > > here. > > > > > > On Fri, Apr 12, 2013 at 7:49 PM, Ted Dunning <[email protected]> > > wrote: > > >> The contract of computeNext is that there are no side effects visible > > >> outside (i.e. apparent functional style). This is required since > > >> computeNext is called from hasNext(). > > >> > > >> We are using a side-effecting style so we have a bug. > > >> > > >> We have two choices: > > >> > > >> a) use functional style. This will *require* that we allocate a new > > >> container element on every call to computeNext. This is best for the > > user > > >> because they will have fewer surprising bugs due to reuse. If > > allocation > > >> is actually as bad as some people think (I remain skeptical of that > > without > > >> tests) then this is a bad move. If allocation of totally ephemeral > > objects > > >> is as cheap as I think, then this would be a good move. > > >> > > >> b) stop using AbstractIterator and continue with the re-use style. > And > > add > > >> a comment to prevent a bright spark from reverting this change. (I > > suspect > > >> that the bright spark who did this in the first place was me so I can > be > > >> rude) > > >
