https://issues.dlang.org/show_bug.cgi?id=13936

--- Comment #30 from Andrei Alexandrescu <andrei@erdani.com> ---
(In reply to hsteoh from comment #27)
> Anyway, another design occurred to me. For ranges with slicing, we could scan Groups in advance and just return slices from .front. The predicate only needs to be evaluated once per element pair. This can be extended to non-sliceable ranges by precomputing the length of each Group in advance: then Group.popFront won't need to evaluate the predicate again (just decrement the length), whereas now if you iterate over 10 copies of each Group, each one will have to evaluate the predicate each time.

I think that's a slightly worse design. Lazy is as lazy does.

--

https://issues.dlang.org/show_bug.cgi?id=13936

--- Comment #31 from Andrei Alexandrescu <andrei@erdani.com> ---
(In reply to hsteoh from comment #28)
> Another note: your current implementation looks like it could be easily extended to handle non-equivalence predicates. All you need to do is to evaluate the predicate on adjacent elements vs. with the head of the group (IOW, just advance groupStart each time in Group.popFront). I doubt it would cause too much performance hit.

That would popFront two ranges instead of one. I still very strongly suggest we define groupBy and groupByAdjancent.

--

https://issues.dlang.org/show_bug.cgi?id=13936

--- Comment #32 from hsteoh@quickfur.ath.cx ---
(In reply to Andrei Alexandrescu from comment #31)
> (In reply to hsteoh from comment #28)
> > Another note: your current implementation looks like it could be easily extended to handle non-equivalence predicates. All you need to do is to evaluate the predicate on adjacent elements vs. with the head of the group (IOW, just advance groupStart each time in Group.popFront). I doubt it would cause too much performance hit.
> 
> That would popFront two ranges instead of one. I still very strongly suggest we define groupBy and groupByAdjancent.

No it won't. You just assign groupNext.save to groupCurrent, then call groupNext.popFront.

--

https://issues.dlang.org/show_bug.cgi?id=13936

--- Comment #33 from Andrei Alexandrescu <andrei@erdani.com> ---
(In reply to hsteoh from comment #32)
> (In reply to Andrei Alexandrescu from comment #31)
> > (In reply to hsteoh from comment #28)
> > > Another note: your current implementation looks like it could be easily extended to handle non-equivalence predicates. All you need to do is to evaluate the predicate on adjacent elements vs. with the head of the group (IOW, just advance groupStart each time in Group.popFront). I doubt it would cause too much performance hit.
> > 
> > That would popFront two ranges instead of one. I still very strongly suggest we define groupBy and groupByAdjancent.
> 
> No it won't. You just assign groupNext.save to groupCurrent, then call groupNext.popFront.

Then I must be misunderstanding something.

--

https://issues.dlang.org/show_bug.cgi?id=13936

--- Comment #34 from Andrei Alexandrescu <andrei@erdani.com> ---
reping @hstehoh jiust because I see him active in the forum and we need to get this and aggregate() done before the next release. @hsteoh please let me know if you want to work on this or I should. Thanks!

--

https://issues.dlang.org/show_bug.cgi?id=13936

--- Comment #35 from hsteoh@quickfur.ath.cx ---
My impression was that you wanted to take this over because you were less than impressed with my implementation of groupBy? Or did I misunderstand your intentions?

--

https://issues.dlang.org/show_bug.cgi?id=13936

--- Comment #36 from Andrei Alexandrescu <andrei@erdani.com> ---
(In reply to hsteoh from comment #35)
> My impression was that you wanted to take this over because you were less than impressed with my implementation of groupBy? Or did I misunderstand your intentions?

Oh, sorry I misunderstood.

I appreciate your work, and I'm not saying this perfunctorily; this important facility has been missing for years until you brought it about, and from a working implementation no less. To put this into perspective, the existing groups() completely sucks.

That said the long and short of it is it's obvious to me that http://dpaste.dzfl.pl/93a13ee08cc1 is the right design; I don't see how the extant implementation or the variants discussed herein have any notable advantage over it, but they do have a bunch of disadvantages.

I was hoping I'd convince you to take it to implementation for forward (and better) ranges whilst keeping your implementation for input ranges. There's also the matter of groupByAdjacent. Please advise, thanks.

--

https://issues.dlang.org/show_bug.cgi?id=13936

--- Comment #37 from hsteoh@quickfur.ath.cx ---
As far as groupByAdjacent is concerned, I am still of the opinion that it belongs as part of groupBy. But since you apparently regard groupBy as release-critical, perhaps it may be wiser to defer this issue to later -- we can always extend groupBy later to encompass the additional functionality, even if it involves unhappy users filing bugs about why groupBy can't handle certain kinds of predicates. Whereas if we release with support for both now, it will be hard to retract one later (at the very least it will involve the dreaded deprecation cycle and consequent unhappy users filing regressions about their predicates breaking). So perhaps for this release we should only include groupBy with support only for equivalence relations, and then we'll have more time to fight it out over whether groupBy should be extended or groupByAdjacent should be introduced. :-P

But, that aside, since you have already written most of the code, I may just copy-n-paste it and fix some of the most egregious flaws in it before submitting it. :-P  No predicting how the reviewers will react, though.

--

https://issues.dlang.org/show_bug.cgi?id=13936

--- Comment #38 from Andrei Alexandrescu <andrei@erdani.com> ---
(In reply to hsteoh from comment #37)
> As far as groupByAdjacent is concerned, I am still of the opinion that it belongs as part of groupBy. But since you apparently regard groupBy as release-critical, perhaps it may be wiser to defer this issue to later -- we can always extend groupBy later to encompass the additional functionality, even if it involves unhappy users filing bugs about why groupBy can't handle certain kinds of predicates. Whereas if we release with support for both now, it will be hard to retract one later (at the very least it will involve the dreaded deprecation cycle and consequent unhappy users filing regressions about their predicates breaking). So perhaps for this release we should only include groupBy with support only for equivalence relations, and then we'll have more time to fight it out over whether groupBy should be extended or groupByAdjacent should be introduced. :-P
> 
> But, that aside, since you have already written most of the code, I may just copy-n-paste it and fix some of the most egregious flaws in it before submitting it. :-P  No predicting how the reviewers will react, though.

Sounds like a plan. Thanks much! I'll be looking for your PR. -- Andrei

--

https://issues.dlang.org/show_bug.cgi?id=13936

--- Comment #39 from hsteoh@quickfur.ath.cx ---
I've run into some roadblocks: RefCounted is @system which makes groupBy @system. This can be mostly swept under the carpet by making the GroupBy ctor @trusted, but there's a further problem: RefCounted is also (inherently) impure, which makes this design unusable in pure code. :-(

--