fix Issue 13441 - joiner asserts with only(x) separator

[CyberShadow]

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

This patch could use review. Is there a better way to do this? Can we expect _currentSep.init.empty to be a valid expression that can be evaluated at compile-time?

Ping @quickfur

[mihails-strasuns]

Can we expect _currentSep.init.empty to be a valid expression that can be evaluated at compile-time?

I doubt so. At least I don't see why it can't be a method relying on run-time data - imagine some range wrapping I/O operations. While conceptually .init.empty sounds like it should be a constant actual implementation may fail CTFE requirements and not account for that special case.

[CyberShadow]

Should I wrap the check in a if(is(typeof(...))) check then?

Edit: All this trouble for an assert, I think at this point it's easier to replace it with a comment.

[monarchdodra]

For what it's worth, I think it's much more reasonable to assume (and expect) that (if its valid) Range.init always be empty. Only does not verify this. And I think it's stupid, as it would produce T.init elements anyways. It makes little sense to me. Making the change to Only would (I think) be a correct move regardless, and just so happen to fix the issue here anyways. It's a 2 line fix too.

When all that is said and done though, arguably, calling anything on a T.init range is wrong, as you can't be sure it was properly run-time initialized anyways. Case in point, Ranges implemented as classes. The sole act of testing .empty would itself be an error...

[CyberShadow]

Makes sense. Replaced with an only fix.

[JakobOvrum]

I don't think changing Only fixes the problem. Calling any range primitives on Range.init is the problem, and changing Only is just a workaround that fixes it for this particular case (that also harms the performance of Only by adding a field)...

edit:

Although I think it's tangential, non-empty default initialized ranges aren't that uncommon, DummyRange is one notable example.

[CyberShadow]

My initial patch called Range.init, it doesn't happen any more.

No new field is needed for OnlyResult, just a constructor.

[JakobOvrum]

Nevermind, I'm not quite used to the new split diff-view on Github yet :)

[monarchdodra]

No new field is needed for OnlyResult, just a constructor.

Could you also make the change in the "higher than 1 arity" version of Only. Let's keep it consistent.

[monarchdodra]

I don't think changing Only fixes the problem.

Agreed, but I think it is a correct change for Only regardless.

hat also harms the performance of Only by adding a field

The field is already there. At worst, it adds a dead initialization in the constructor, which should optimized away anyways.

[CyberShadow]

Could you also make the change in the "higher than 1 arity" version of Only. Let's keep it consistent.

Right, didn't realize it needed a fix too. Done.

[monarchdodra]

Edit: All this trouble for an assert, I think at this point it's easier to replace it with a comment.

Technically, if you replace the code with:

import std.algorithm;
import std.range;
import std.stdio;

void main()
{
    int[][] lines = [[1], [2], [3]];
    lines
        .joiner(only(7)).writeln();
}

You'd see the output: [0, 1, 7, 2, 7, 3]. Notice that first 0. So no assert, but the result is still wrong...

[CyberShadow]

Ah, good catch. Didn't think joiner actually depended on the initial state of empty.

[monarchdodra]

Though it doesn't completely fix the issue, I is still think it's a valid change, so LGTM. Also pinging @Poita: Afaik, both you and @JakobOvrum wrote Only. Do you guys agree with this change?

[JakobOvrum]

Yeah, looking again, I think the change to OnlyResult is really nice :)

[mihails-strasuns]

Yeah I like the new diff too

[monarchdodra]

Auto-merge toggled on