May 23, 2016
On Monday, 23 May 2016 at 11:52:35 UTC, Seb wrote:
> On Sunday, 22 May 2016 at 18:56:30 UTC, qznc wrote:
>> On Monday, 4 March 2013 at 19:11:17 UTC, Steven Schveighoffer wrote:
>>> [...]
>>
>>
>> Below is an implementation, which matches MySplitter with dmd, but not with ldc. More precisely:
>>
>> [...]
>
> have you thought about opening a PR to improve `splitter`?

Yes, but I'm not sure about the goals.

I also want to dig a little deeper. Being "sometimes faster" without some understand why and when feels unsatisfying.

Additionally, there is this weird special case for a bidirectional range, which just adds unnecessary overhead. Is "remove dead code" a good enough reason in itself for a PR?
May 23, 2016
On Monday, 23 May 2016 at 12:01:52 UTC, qznc wrote:
> On Monday, 23 May 2016 at 11:52:35 UTC, Seb wrote:
>> On Sunday, 22 May 2016 at 18:56:30 UTC, qznc wrote:
>>> On Monday, 4 March 2013 at 19:11:17 UTC, Steven Schveighoffer wrote:
>>>> [...]
>>>
>>>
>>> Below is an implementation, which matches MySplitter with dmd, but not with ldc. More precisely:
>>>
>>> [...]
>>
>> have you thought about opening a PR to improve `splitter`?
>
> Yes, but I'm not sure about the goals.
>
> I also want to dig a little deeper. Being "sometimes faster" without some understand why and when feels unsatisfying.

Good luck. If you need help, it's probably the best to open the PR as people can directly see your code and the Phobos repo is usually pretty active.

> Additionally, there is this weird special case for a bidirectional range, which just adds unnecessary overhead.

> Is  "remove dead code" a good enough reason in itself for a PR?

Absolutely ;-)
May 23, 2016
On Monday, 23 May 2016 at 12:01:52 UTC, qznc wrote:
> Additionally, there is this weird special case for a bidirectional range, which just adds unnecessary overhead. Is "remove dead code" a good enough reason in itself for a PR?

Forget the "dead code comment" it is a actually a missing feature. In the case the separator is a range and the input range is bidirectional, the splitter result should be bidirectional as well. It is not, because the implementation of back() and popBack() is missing, although some bookkeeping code for it exists.
May 23, 2016
On 05/23/2016 08:17 AM, qznc wrote:
> On Monday, 23 May 2016 at 12:01:52 UTC, qznc wrote:
>> Additionally, there is this weird special case for a bidirectional
>> range, which just adds unnecessary overhead. Is "remove dead code" a
>> good enough reason in itself for a PR?
>
> Forget the "dead code comment" it is a actually a missing feature. In
> the case the separator is a range and the input range is bidirectional,
> the splitter result should be bidirectional as well. It is not, because
> the implementation of back() and popBack() is missing, although some
> bookkeeping code for it exists.

Most uses are forward, so perhaps it's best to eliminate the bidirectional bookkeeping if it adds overhead, and confine those to a separate function e.g. splitterBidirectional. There is precedent for that in Phobos. -- Andrei
May 23, 2016
On Monday, 23 May 2016 at 13:20:55 UTC, Andrei Alexandrescu wrote:
> Most uses are forward, so perhaps it's best to eliminate the bidirectional bookkeeping if it adds overhead, and confine those to a separate function e.g. splitterBidirectional. There is precedent for that in Phobos. -- Andrei

I don't see the value add that things like filterBidirectional give when the back and popBack functions can be static if-ed in or out the normal filter. filterBidirectional always felt like a relic of a time when static if's didn't exist, even though I know that's not the case.
May 23, 2016
On 5/23/16 9:44 AM, Jack Stouffer wrote:
> On Monday, 23 May 2016 at 13:20:55 UTC, Andrei Alexandrescu wrote:
>> Most uses are forward, so perhaps it's best to eliminate the
>> bidirectional bookkeeping if it adds overhead, and confine those to a
>> separate function e.g. splitterBidirectional. There is precedent for
>> that in Phobos. -- Andrei
>
> I don't see the value add that things like filterBidirectional give when
> the back and popBack functions can be static if-ed in or out the normal
> filter.

What would be the condition tested in the static if? Recall that sometimes you do want a forward range even when you could define a bidirectional range. -- Andrei

May 23, 2016
On Monday, 23 May 2016 at 14:17:59 UTC, Andrei Alexandrescu wrote:
> What would be the condition tested in the static if?

I would assume `static if (isBidirectionalRange!Range)`

> Recall that sometimes you do want a forward range even when you could define a bidirectional range. -- Andrei

I honestly can't think of a time when would this be the case. What are you losing by having back and popBack defined?
May 23, 2016
On Monday, 23 May 2016 at 14:25:52 UTC, Jack Stouffer wrote:
> On Monday, 23 May 2016 at 14:17:59 UTC, Andrei Alexandrescu wrote:
>> What would be the condition tested in the static if?
>
> I would assume `static if (isBidirectionalRange!Range)`
>
>> Recall that sometimes you do want a forward range even when you could define a bidirectional range. -- Andrei
>
> I honestly can't think of a time when would this be the case. What are you losing by having back and popBack defined?

Here is a commit, which removes the dead code:

https://github.com/qznc/phobos/commit/0cb1e70f4504a52b81c30342be7ec26d597fac69

It would be strictly better to implement the back and popBack, no?

My single test case says removing the code does not result in faster code. Maybe dmd optimizes the bookkeeping away.

I see three options:

1. Remove dead bookkeeping code
2. Implement back() and popBack()
3. Use alternative splitter implementation (and implement back() and popBack())

The third one would be the best, if it is really faster.
May 23, 2016
On 05/23/2016 10:47 AM, qznc wrote:
> It would be strictly better to implement the back and popBack, no?

If it adds no overhead, you're right. -- Andrei
May 23, 2016
On 05/23/2016 10:25 AM, Jack Stouffer wrote:
> On Monday, 23 May 2016 at 14:17:59 UTC, Andrei Alexandrescu wrote:
>> What would be the condition tested in the static if?
>
> I would assume `static if (isBidirectionalRange!Range)`
>
>> Recall that sometimes you do want a forward range even when you could
>> define a bidirectional range. -- Andrei
>
> I honestly can't think of a time when would this be the case. What are
> you losing by having back and popBack defined?

You might be doing extra work even if the user never calls back and popBack. -- Andrei