[Static if] replace overload constraints with static if (iteration.d)

[wilzbach]

Split up of #5145

[wilzbach]

I grouped the overloads into byValue and possibly byReference (aka auto ref)

[wilzbach]

Technically this is a "breaking" change, but IIRC non-documented members can be set to private without deprecation path is the user can only know about them by reading the code on his own and no guarantees were given.

[andralex]

I came to see label: as a near antipattern - it's too nonlocal and makes code difficult to understand and review without context. @wilzbach how about a rule that prevents at least the use of private: and public: in phobos?

[wilzbach]

This change groups the first and third overload and the second + fourth, respectively as the latter ones might have their input as reference. So this overload is for calling with args given by-Value:

[quickfur]

Did you mean the 1st and 4th overloads are grouped as one group, and the 2nd and 3rd as another group? I'm looking at the code and the sig constraints of the 1st and 3rd, taken together, don't match what you have here.

[quickfur]

Assuming you meant that the 1st and 4th overloads are grouped together, the code looks OK to me.

[quickfur]

The code itself looks good, but I feel it falls short of the goal. While we have reduced 4 overloads to 2, as far as the user's POV is concerned, esp. wrt. the ddocs, these overloads are still somewhat inscrutable.

For example, the first overload's sig constraints refers to isRangeIterable, which is now not only an undocumented trait, but a private one. Furthermore, there's the ugly __traits(compiles, ...) the intent of which is unclear.

Taking a step back, let's look at this entire overload set. AIUI the reason we can't put everything into a single overload is because of the auto ref in the second group, an understandable distinction. The primary reason for this distinction seems to be that one group deals with things that support foreach whereas the other group deals with things that don't support foreach but do at least have a range API.

So from the user's POV, the sig constraints really should look something like this:

void each(Range)(Range r)
    if (!isForeachIterable!Range && isInputRange!Range)
    { ... }

void each(Range)(auto ref Range r)
    if (isForeachIterable!Range)
    { ... }

(And on that note, the original isRangeIterable trait is misleadingly named, because what it really means is "range-iterable with a single index". And the funny-looking __traits(compiles...) check appears to be trying to ensure that any subranges aren't infinite or have unknown length, which would make it impossible to be passed to pred.)

I would say those should be the public-facing sig constraints, and inside the overload bodies, the static ifs must check for all constraints pertaining to that code block, especially in the else branch, and there should be a final else assert(0,...); to catch any cases that the current implementation doesn't support, with of course an appropriate error message indicating what went wrong.

[quickfur]

Assuming you meant that the 1st and 4th overloads are grouped together, the code looks OK to me.

[quickfur]

The code for the second group checks out as well (assuming you meant they correspond with the 2nd and 3rd overloads in the original).

[quickfur]

And just for the record, the last bit is what I meant in various forum posts that user-facing overloads should have sig constraints that reflect intent rather than implementation: the intent of each is that it would accept anything that either supports foreach or has a range API. So those should be what constitute the sig constraints of each.

There may be, however, some ranges that satisfy these constraints but which aren't handled by the current implementation; these should be caught by the final else clause in the static if inside each overload body, with a helpful error message explaining to the user why it didn't work ("support for range with characteristics XYZ isn't implemented yet", or "cannot call each with range with characteristics XYZ because PQR").

[andralex]

noice

[andralex]

I came to see label: as a near antipattern - it's too nonlocal and makes code difficult to understand and review without context. @wilzbach how about a rule that prevents at least the use of private: and public: in phobos?

[andralex]

Jenkins doesn't build and clicking the link goes to

503 Service Unavailable

No server is available to handle this request.

[jmdavis]

I came to see label: as a near antipattern - it's too nonlocal and makes code difficult to understand and review without context. @wilzbach how about a rule that prevents at least the use of private: and public: in phobos?

I would point out that aside from the fact that druntime and Phobos use private: and public: in quite a few places, the trend for handling attributes seems to be going in the opposite direction of what you're suggesting. Increasingly, we seem to be ending up with attributes being applied with : or braces rather than applied to each symbol individually. And of course, using braces has pretty much the same problems that using : does. I don't know how easy it would be to catch either.

Now, I tend to agree that mass-applying attributes is a problem, but to be honest, I think that using public: and private: causes far less of a problem than mass-applying other attributes, particularly since it makes a lot of sense to be grouping public and private stuff that way, whereas which of the other attributes apply to functions generally has nothing to do with how you would organize them in the file.

Regardless, for better or worse, the current trend seems to be headed in the opposite direction of avoiding : and braces with attributes - I think mostly because folks don't want to have to keep typing them. So, if we want to make a policy change, we should keep that in mind.

[CyberShadow]

@wilzbach I noticed that this (nonsensical) example from issue 15459 doesn't compile any more, do you think it indicates a regression? I couldn't come up with a meaningful variation that would manifest as a regression.

import std.stdio;
import std.algorithm;

char somefunc(char c)
{
    return c;
}

void main()
{
    stdin
        .byLine
        .each!(map!somefunc);
}

[wilzbach]

do you think it indicates a regression? I couldn't come up with a meaningful variation that would manifest as a regression.

Hmm, auto-decoding has been a thing in Phobos for a long, long time - so no I don't think it's a regression. On the contrary, now at least you get a compiler error.
With 2.073.1

bar.o:bar.d:function _D3std9algorithm9iteration30__T3mapS19_D3bar8somefuncFaZaZ11__T3mapTAaZ3mapFNaNbNiNfAaZS3std9algorithm9iteration39__T9MapResultS19_D3bar8somefuncFaZaTAaZ9MapResult: error: undefined reference to '_D3std9algorithm9iteration39__T9MapResultS19_D3bar8somefuncFaZaTAaZ9MapResult6__ctorMFNaNbNcNiNfAaZS3std9algorithm9iteration39__T9MapResultS19_D3bar8somefuncFaZaTAaZ9MapResult'
collect2: error: ld returned 1 exit status
Error: linker exited with status 1

With 2.074.0

/usr/include/dlang/dmd/std/algorithm/iteration.d(890):        std.algorithm.iteration.each!(map).each(Range)(Range r) if (!isForeachIterable!Range && (isRangeIterable!Range || __traits(compiles, typeof(r.front).length)))
/usr/include/dlang/dmd/std/algorithm/iteration.d(925):        std.algorithm.iteration.each!(map).each(Iterable)(auto ref Iterable r) if (isForeachIterable!Iterable || __traits(compiles, Parameters!(Parameters!(r.opApply))))

[CyberShadow]

Right, but is the compiler error fixing (correctly replacing) the link error, or just making it unreproducible with that example?