On 2/14/2017 8:25 AM, Andrei Alexandrescu wrote:
> Range remove
> (SwapStrategy s = SwapStrategy.stable, Range, Offset...)
> (Range range, Offset offset)
> if (s != SwapStrategy.stable
>     && isBidirectionalRange!Range
>     && hasLvalueElements!Range
>     && hasLength!Range
>     && Offset.length >= 1);
>
> The function name is on the first line.

It could be improved slightly using indentation:

Range remove
    (SwapStrategy s = SwapStrategy.stable, Range, Offset...)
    (Range range, Offset offset)
    if (s != SwapStrategy.stable
        && isBidirectionalRange!Range
        && hasLvalueElements!Range
        && hasLength!Range
        && Offset.length >= 1);

But there's another issue here. remove() has other overloads:

Range remove
    (SwapStrategy s = SwapStrategy.stable, Range, Offset...)
    (Range range, Offset offset)
    if (s == SwapStrategy.stable
        && isBidirectionalRange!Range
        && hasLvalueElements!Range
        && Offset.length >= 1)

Range remove(alias pred, SwapStrategy s = SwapStrategy.stable, Range)
    (Range range)
    if (isBidirectionalRange!Range
        && hasLvalueElements!Range)

Two constraints are common to all three, those are the only ones that actually need to be in the constraint. The others can go in the body under `static if`, as the user need not be concerned with them.

This is a general issue in Phobos that too many constraints are user-facing when they don't need to be.

A further improvement in the documentation would be to add links to isBidirectionalRange and hasLvalueElements.

On 2/14/2017 12:46 PM, Walter Bright wrote:
> A further improvement in the documentation would be to add links to
> isBidirectionalRange and hasLvalueElements.

For reference:

http://dlang.org/phobos/std_algorithm_mutation.html#.remove

On Tuesday, 14 February 2017 at 16:25:17 UTC, Andrei Alexandrescu wrote:
> Range remove
> (SwapStrategy s = SwapStrategy.stable, Range, Offset...)
> (Range range, Offset offset)
> if (s != SwapStrategy.stable
>     && isBidirectionalRange!Range
>     && hasLvalueElements!Range
>     && hasLength!Range
>     && Offset.length >= 1);
>

Range
remove (SwapStrategy s = SwapStrategy.stable, Range, Offset...)
(Range range, Offset offset)
if CanRemove!(s, range, offset);

> The function name is on the first line. I think 10 seconds would be an exaggeration of an admittedly real issue.
>

Thought so, too, but then I *did* spend 10 seconds trying to find it myself!

> Tuple!(InputRange1, InputRange2)
> swapRanges(InputRange1, InputRange2)(InputRange1 r1, InputRange2 r2)
> if (isInputRange!(InputRange1) && isInputRange!(InputRange2)
>     && hasSwappableElements!(InputRange1)
>     && hasSwappableElements!(InputRange2)
>     && is(ElementType!(InputRange1) == ElementType!(InputRange2)));
>

Tuple!(InputRange1, InputRange2)
swapRanges(InputRange1, InputRange2)(InputRange1 r1, InputRange2 r2)
if CanSwapRanges!(InputRange1, InputRange2);

On Tue, Feb 14, 2017 at 12:46:17PM -0800, Walter Bright via Digitalmars-d wrote: [...]
> Range remove
>     (SwapStrategy s = SwapStrategy.stable, Range, Offset...)
>     (Range range, Offset offset)
>     if (s != SwapStrategy.stable
>         && isBidirectionalRange!Range
>         && hasLvalueElements!Range
>         && hasLength!Range
>         && Offset.length >= 1);
> 
> But there's another issue here. remove() has other overloads:
> 
> Range remove
>     (SwapStrategy s = SwapStrategy.stable, Range, Offset...)
>     (Range range, Offset offset)
>     if (s == SwapStrategy.stable
>         && isBidirectionalRange!Range
>         && hasLvalueElements!Range
>         && Offset.length >= 1)
> 
> Range remove(alias pred, SwapStrategy s = SwapStrategy.stable, Range)
>     (Range range)
>     if (isBidirectionalRange!Range
>         && hasLvalueElements!Range)
> 
> Two constraints are common to all three, those are the only ones that actually need to be in the constraint. The others can go in the body under `static if`, as the user need not be concerned with them.
> 
> This is a general issue in Phobos that too many constraints are user-facing when they don't need to be.

+1.  I've raised this point many times, but didn't seem to generate much response.  Basically, there are two more-or-less equivalent ways of having multiple implementations of a function based on its arguments: (1) via signature constraints, and (2) via static if's inside the function (or template) body.

>From an implementor's POV, (1) is more desirable, because it's easy to
add a new overload when you need to extend the function to handle new types. However, from a user's POV, (2) is much more friendly -- one only needs to look at the old overloads of std.conv.toImpl to see why: it was an obtuse mass of only slightly different sig constraints over a ridiculous number of overloads, each intended to work with very specific argument combinations, all of which are implementation details the user need not know about.

Thankfully, the docs for std.conv.to have been greatly improved since the last time I had to work with the code -- everything is now consolidated under a single template function std.conv.to, and the implementation details are hidden behind module-private overloads. This is the way it should be.

I argue that the same pattern should be applied to (most of the) other overloaded Phobos functions as well, such as remove() shown above. There really should only be *one* remove() function with the two constraints Walter indicated, and the current overloads should either be refactored under static if's inside the function body, or else renamed and made into module-private implementation functions called by remove(). Multiple user-facing overloads really only should be used where each overload represents a *conceptually-different* aspect of the function (which should be relatively rare).

In general, if a template function (or set of overloads) conceptually does the same thing, it should be made into a *single* function. If the implementation differs, that's the problem of the Phobos maintainers, and should be handled as module-private symbols forwarded to by the single user-facing function.

Another bonus to this approach is that it allows us to customize nicer error messages via static assert if none of the (internal) overloads match the passed argument types. E.g., if the user passes a swappable bidirectional range to remove() but for whatever reason the arguments fail to match any of its implementations, then instead of spewing out pages of inscrutable encrypted Klingon (argument types XYZ do not match any overloads of remove(), candidates are: [... snip 5 pages of unreadable function signatures...]), we can, say, do something like:

	static if (...)
		return removeImplA(args);
	else static if (...)
		return removeImplB(args);
	...
	else
		static assert("Arguments to remove() must satisfy conditions X, Y, Z [insert explanation here]");

That's a 1-line error message that tells the user exactly what went wrong, instead of 5 pages of 10-line function signatures that the user must parse and then mentally evaluate each Boolean condition for, in order to deduce exactly why the passed arguments didn't work.


> A further improvement in the documentation would be to add links to isBidirectionalRange and hasLvalueElements.

This is a good idea. +1.


T

-- 
"I'm running Windows '98." "Yes." "My computer isn't working now." "Yes, you already said that." -- User-Friendly

On Tuesday, 14 February 2017 at 20:03:13 UTC, Jonathan M Davis wrote:
> That being said, at some point, you have to ask whether each added feature is worth the cost when you consider how it's going to clutter up function signatures even further. And while I do think that there is value in DIP 1005 and the proposed from template, I also think that it's taking it too far. IMHO, it's just not worth marking functions even further - at least not in most code. Maybe it's worth it in something like Phobos where everyone is using it and benefiting from the compilation speed up, but Walter has been wanting to implement lazy imports anyway, and that would fix the problem without doing anything to any function signatures. It does lose the benefits of tying the imports to the function, but personally, I don't think that that's's worth the extra cost of further cluttering up the function signature. As it is, I'm increasingly of the opinion that local and selective imports aren't worth it. It's just so much nicer to able to slap the required imports at the beginning of the module and forget about them than having to worry about maintaining a list of selective imports or have all of the extra import lines inside of all of the functions. And adding imports to the function signatures is just making the whole local import situation that much worse.

+1. D's beautiful syntax plays a key role for attracting new folks, and I see it endangered by recent developments.

On 2/14/2017 2:01 PM, H. S. Teoh via Digitalmars-d wrote:
> This is a good idea. +1.

Please take this on!

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

On Tue, 14 Feb 2017 21:33:00 +0000, Lurker wrote:

> On Tuesday, 14 February 2017 at 16:25:17 UTC, Andrei Alexandrescu wrote:
>> Range remove (SwapStrategy s = SwapStrategy.stable, Range, Offset...)
>> (Range range, Offset offset)
>> if (s != SwapStrategy.stable
>>     && isBidirectionalRange!Range && hasLvalueElements!Range &&
>>     hasLength!Range && Offset.length >= 1);
>>
>>
> Range remove (SwapStrategy s = SwapStrategy.stable, Range, Offset...)
> (Range range, Offset offset)
> if CanRemove!(s, range, offset);

Which adds another layer to determine what sort of arguments the thing requires. Fewer layers is better.

In the past, I tried to track down compilation errors in Phobos (due to my changes) relating to, I think, template overload selection in std.conv based on template constraints. It was hell. Multiple layers with `static if (__traits(compiles))` interspersed.

I'm not too keen on template constraints in general. If you need them, keep them short and keep the entire definition in one place.

>> The function name is on the first line. I think 10 seconds would be an exaggeration of an admittedly real issue.
>>
>>
> Thought so, too, but then I *did* spend 10 seconds trying to find it myself!

Does your editor not have syntax highlighting? For me, I just look immediately below the giant block of blue comment text and check the second word in the line below.

On Tue, 14 Feb 2017 19:08:53 +0000, bachmeier wrote:
> I am not familiar with all of the past discussion of this issue, but something that I have wondered is why we can't do something like
> 
> alias fooConstraint = (s != SwapStrategy.stable
>    && isBidirectionalRange!Range && hasLvalueElements!Range &&
>    hasLength!Range && Offset.length >= 1);
> 
> Range remove
>    (SwapStrategy s = SwapStrategy.stable, Range, Offset...)
>    (Range range, Offset offset) if fooConstraint;

Because now I have to look up the definition of fooConstraint and then I have to look up the definition of each element within it.

If the entirety of the constraint is in the body of that helper template, that's a step up from what we have today. But that would involve code duplication, which people tend to dislike.

On Tuesday, 14 February 2017 at 20:46:17 UTC, Walter Bright wrote:
> A further improvement in the documentation would be to add links to isBidirectionalRange and hasLvalueElements.

Kneel before your god!

http://dpldocs.info/experimental-docs/std.algorithm.mutation.remove.1.html

Take a look at this insane automatic cross referencing:

http://dpldocs.info/experimental-docs/std.range.chain.html


My ref thing still isn't perfect, but dpldocs is already streets ahead of anything dlang.org has to offer and continues to make bursts of strides.

On 2017-02-15 06:28, Adam D. Ruppe wrote:

> Kneel before your god!
>
> http://dpldocs.info/experimental-docs/std.algorithm.mutation.remove.1.html
>
> Take a look at this insane automatic cross referencing:
>
> http://dpldocs.info/experimental-docs/std.range.chain.html
>
>
> My ref thing still isn't perfect, but dpldocs is already streets ahead
> of anything dlang.org has to offer and continues to make bursts of strides.

Your documentation is an improvement but it doesn't help when reading the source code.

-- 
/Jacob Carlborg