New implementation of groupBy #2878
Conversation
|
I see dead unittests. |
|
Oh, you mentioned it... |
|
You can get the unittests to run by commenting out |
|
Oh, speaking of which, is anybody actually looking into fixing |
|
Between this and RefCounted, I wouldn't want one to be blocked by another. I suggest we make unittests unsafe for now and file an issue for RefCounted. |
|
I don't think that's going to go down well. The current groupBy implementation, for all of its flaws and inefficiencies, does fully support I think the correct solution is to work on |
|
There is no user for the current groupBy so no breakage. We will work on RefCounted but it should be fine to release this with less guarantees and extend them later. If you're okay with letting this flap in the wind until the other is fixed, that's fine too but suboptimal (bit rot etc). May be good as a motivator though. |
|
You're right, while there are users who follow git HEAD (bearophile, for example), it's not an official release so we are not obligated to support it. I'll comment out the failing attributes. |
|
ping @andralex |
|
Rebased on new split std.algorithm package. |
Restrict old groupBy implementation to input-only ranges. Add unittest. Temporarily disable failing attributes, until RefCounted is repaired to support `@safe nothrow pure`.
|
Fixed compile error and rebased. |
|
ping @andralex The autotester is green now. Are we merging this or not? |
| } | ||
| } | ||
| else | ||
| this(Range _r, ElementType!Range _prev) |
There was a problem hiding this comment.
Well, the usual convention is members are _-prefixed and parameters aren't...
|
Nice. The nitpicks are not important so if you're not around I'll pull this in 30 minutes or so and leave them to a future pass. Thanks for this work!! |
|
All done. |
New implementation of groupBy
|
Fantastic. Thanks! |
Fixes: https://issues.dlang.org/show_bug.cgi?id=13936
Basically, rip out support for non-equivalence ranges (for now) and restrict the current
groupByimplementation to input ranges only. Forward ranges and higher will use Andrei's proposed design that avoids the need topopFrontthrough each subrange more than once in a linear traversal of the underlying range.However, the new code is not expected to pass the autotester just yet, because of blocking issues with
RefCountedbeing neither@safe,pure, nornothrow.