std.range package

[9il]

Needs #2684.

Issue 13253 - use more scoped imports in phobos
Please, freeze all other std.range commits!
Known PR that should be blocked:

1. Optimization for put/doPut templates that use outputRanges that should be pass by value #2655
2. Mark methods in std.range.Cycle for static array as system #2647
3. Introduce std.range.generate #2606
4. fix Issue 12409 - Add "each" function as found in Ruby and jQuery #2024 (only one comment line, can be merged)

std.range

Contains D ranges (zip, iota etc.).

Global imports

public import std.range.constraints;
public import std.range.interfaces;
public import std.array;
public import std.typecons : Flag, Yes, No;

import std.traits;
import std.typetuple;

std.range.constraints

Contains range has*, is*Range primitives and convenience functions for
manipulating ranges (like put).

Global imports

public import std.array : empty, save, popFront, popBack, front, back;
import std.traits;

I think the next PR will move empty, save, popFront, popBack, front, back into std.range.constraints.
The reason is to don't import std.array globally to all Phobos modules.
std.range \ std.range.constraints is imported in std.array anyway.

All scoped imports

template hasSwappableElements(R)
{
    import std.algorithm : swap; 
    ...

//Helper function to handle chars as quickly and as elegantly as possible
private void putChar(R, E)(ref R r, E e)
if (isSomeChar!E)
{
    ...
    static if ((wsCond && E.sizeof < wchar.sizeof) || (dsCond && E.sizeof < dchar.sizeof))
    {
        ...
    }
    else static if (wsCond || csCond)
    {
        import std.utf : encode;
        ...
    }
    else static if (wcCond || ccCond)
    {
        import std.encoding : encode;
        ...
    }

ElementType!R moveFront(R)(R r)
{
    static if (is(typeof(&r.moveFront))) {
        return r.moveFront();
    } else static if (!hasElaborateCopyConstructor!(ElementType!R)) {
        return r.front;
    } else static if (is(typeof(&(r.front())) == ElementType!R*)) {
        import std.algorithm : move; //<<<<<<<<<<<
        ...
///ditto 
ElementType!R moveBack(R)(R r)
///ditto 
ElementType!R moveAt(R)(R r, size_t index)

std.range.interfaces

Contains interfaces and classes.

Global imports

import std.range.concept;
import std.traits;
import std.typetuple;

All scoped imports

// CTFE function that generates mixin code for one put() method for each
// type E.
private string putMethods(E...)()
{
    import std.conv : to;
    ...

See also: #2765

[9il]

ping authors (@jmdavis, @dsimcha) and @MartinNowak

[MartinNowak]

That's a really good idea.
We must be very careful with this to not break any code though.
So the new idiom would be.

import std.range.concepts;

void foo(R)(R r) if (isForwardRange!R)
{
    import std.range; // import rest
    copy(r.retro, r);
}

[9il]

There is only one change (except imports and private _min) I have made: package doPut became private doPut. Only std.encoding used doPut so I just rename it. doPut is called internally in put.
All other phobos modules compile without changes. Phobos like a big unittest.

There are few links in comments, that can be broken. I will check them. But comments checks can be done after merge.
The reason to merge this PR as soon as possible is that other std.range commits should be blocked by this PR.

So there is main thing to review: std.range.concept and std.range.interfaces name conversion.

[DmitryOlshansky]

This is something I tried to do long ago, so it's awesome to see this idea implemented.

I would warn about using selective imports at global scope, these still create a bunch of aliases and thus leak symbols out of module. (I might be wrong, just check if any of them are accessible outside of std.range)

[quickfur]

Very nice! Let's get this into shape for merging, and then we should start looking into splitting up std.algorithm as well.

For that latter case, since it's so huge, maybe we can do it piecemeal instead of all in one go. (I've tried to do it in one go before, but the effort involved is too much for a single PR and besides risks conflicts with other std.algorithm PRs that may get merged in the meantime.) The easiest pieces to handle are those functions that are frequently used throughout Phobos, like find, startsWith, etc.. Perhaps those can be factored out first, leaving the remaining code in package.d, then afterwards we can move more stuff out until nothing is left in package.d except public imports of the new submodules.

[mihails-strasuns]

I think name concept is not truly appropriate here. It is more like std.range.traits

[9il]

concept also includes stuff like put and moveFront. Is traits name is appropriate?

[mihails-strasuns]

Probably constraints is even more fitting as this is the term used in D for template signature conditions. Problem with concept is that it has strong C++ connotations and is not defined in D in any right now.

What about separating stuff like put in dedicated module, something like std.range.ufcs?

[9il]

@Dicebot

1. OK
2. There is no any profit with dedicated module because stuff like put is used in traits.

[monarchdodra]

There was a reason for doPut here over put. Why did you feel this needed change?

[9il]

doPut was package function, but it was not accessible from std.encoding. It was accessible only from std.range.* since it in std.range.constraints.

[monarchdodra]

Edit: Hum... actually, you kept the isNativeOutputRange, so that works. Never mind, cary on.

[monarchdodra]

doPut was package function, but it was not accessible from...

That alone doesn't justify the change though, they don't have the same semantics! But in this case, it still works.

[9il]

Yes, I checked it )

[monarchdodra]

I'm still not a huge fan of having "put" or "moveFront" be part of "constraints". I'd have loved to have a those in "concept", which might have actually publicly included "constraints". More often that not, you don't need these functions for constraint checking. If you start writting is(typeof(put(r, e))), then you should now you need range concepts.

@Dicebot suggested calling concept as ufcs, but it kind of turns out that in generic code, it's kind of an anti-pattern. You should call things like put in a non-ufcs pattern...

[monarchdodra]

Oh, shit. Don't do this. If somebody misses a dependency in code, all unittests will pass, we will ship it, and clients' code won't compile. It's happened before it is an awful kind of bug.

Use a mixin if you must, but please avoid "version(unittest)" in global space at all cost.

[9il]

Fixed, see my last comment #2675

[monarchdodra]

I'm not sure how it's fixed? We still have global imports that are only included in unittests. We are currently unable to catch an error where code that depends on these failed to do a proper import.

IMO, this is worst than an unconditional import.

[9il]

This imports have been fixed in #2675.

[monarchdodra]

I'm not talking about the imports. They may or may not be correct. Heck, it's not about the imports, it's about the functions that depend on them.

The issue is that we currently aren't covering whether or not our standard non-unittest functions have the correct imports. As it stands, especially with your imports cleaning, we have absolutely no way to know if any of these functions in range had an actual dependency on algorithm, which you forgot to locally include. Or somebody else in the future might write a function that requires std.algorithm.

We will fail to compile for literally every client, and not even see it.

It should be kept as simple global import, or scoped import. version(unittest) is evil and dangerous.

[9il]

Move dummy ranges into std/internal/unittest/dummyrange.d : see #2691

[9il]

version(unittest) is a huge headache ...

[mihails-strasuns]

Proof? You can just make the imports unittest-local, can't you?

Not always. If you have some additional symbols / function shared by unittest blocks there is no place to put imports for those other than in same version(unittest) block. However second proposal for requiring forced namespace hygiene may be enough to address it.

[monarchdodra]

If these "additional symbols / function shared by unittest blocks" only make sense for unittests, then version(unittest) is not that much of a problem, since actual code can't "accidently" depend on those.

[9il]

stuff from version(unittest) scope accessible from all functions.

[monarchdodra]

Very good work. Sorry for being late for the review. I think there might still be some issues to discuss, but the overall change is nice. And I think at this point, merge than argue is a better approach. We still have until the next release to tweak.

[mihails-strasuns]

I'm still not a huge fan of having "put" or "moveFront" be part of "constraints". I'd have loved to have a those in "concept", which might have actually publicly included "constraints".

I agree that it is sub-optimal to keep those with constraints but concepts are completely irrelevant here. Those primitives simply provide default range implementations for existing types (though you are right that that does not imply UFCS). std.range.defimpl? :)

[MartinNowak]

What about separating stuff like put in dedicated module, something like std.range.ufcs?

I think that should still be done for a cleaner package structure.

[9il]

But when you use isInputRange you always use front (ufcs method for arrays in std.range.constraints). So you generally always need to import two modules. Programmers is lazy and they would import all package std.range instead of std.range.traits and std.range.methods.

[MartinNowak]

But when you use isInputRange you always use front (ufcs method for arrays in std.range.constraints). So you generally always need to import two modules. Programmers is lazy and they would import all package std.range instead of std.range.traits and std.range.methods.

Sure, but you can public import std.range.methods; in std.range.traits.
And please rename std.range.constraints to std.range.traits as that is the established term (in D and C++) for what the module provides.

[9il]

Ping any other collaborator to confirm suggestion "rename std.range.constraints to std.range.traits."
I need common dissigion from two collaborators. First is @MartinNowak .

[quickfur]

I think on the forum we agreed on std.range.primitives?

[quickfur]

Or maybe we can have both, with template constraints in std.range.traits and things like array range primitives in std.range.primitives. But it might be a bit too early to split things up so finely.

[9il]

Please split two questions:

1. Renaming existing module
2. Splitting

@quickfur Can you give a link to forum discussion?

[quickfur]

Sorry, I use the mailing list interface so I didn't think to post the forum link. Here it is: http://forum.dlang.org/thread/mailman.1974.1415943181.9932.digitalmars-d@puremagic.com

[9il]

OK. There is three variants:

1. std.range.constraints @klickverbot (@9il not collaborator)
2. std.range.primitives @quickfur @Dicebot @DmitryOlshansky @klickverbot (update)
3. std.range.traits @MartinNowak

Collaborator, please select one!

@MartinNowak (update) @monarchdodra and @Dicebot want to split.
But any way I don't want to split current std.range.constraints into two parts:

1. When you use isInputRange you always use front (ufcs method for arrays in std.range.constraints). So you generally always need to import two modules. Programmers is lazy and they would import all package std.range instead of std.range.traits and std.range.methods. (you can read @MartinNowak comment above).
2. Both parts needs to import each other anyway.

Please do not split module for beautiful names.

[dnadlinger]

Oh, I'm not arguing for constraints, I don't know where you got that from. In fact, I'm rather strongly in the primitives camp.

I also think popFrontN and the other convenience wrappers shouldn't be in the primitives module.

[9il]

@klickverbot I got it from forum.Voting list is updated.

[MartinNowak]

Please do not split module for beautiful names.

No, we split modules so that people reading the docs or contributing code know where to find stuff and having popFront* and such in a module called constraints is clearly misleading.

[quickfur]

@9il It's not true that when you use isInputRange you always use array.front. I have lots of code that uses isInputRange but the ranges they process are not arrays. So strictly speaking, there is no real dependency between the two.

The way I see it, the split should more-or-less be along the same (or similar) lines as the current std.range documentation's sections:

1. The first two sections contain template constraints, and make sense to be grouped together (isInputRange, hasLength, etc.). I'd say the templates here should go in std.range.primitives.
2. The 3rd section contains the heavier-weight range wrappers and compositors (stride, cycle, take, iota, etc.), so they should go in their own submodule (we don't have to do this for the next release; probably it's OK to leave them in std/range/package.d for now).
3. The 4th section contains runtime-polymorphic range wrappers (InputRange, InputRangeObject, etc.), and can also be an independent submodule -- IME they are rarely needed except when you need to have runtime polymorphism of ranges. So they could go into std.range.interfaces.
4. The 5th section contains convenience methods like popFrontN, moveFront, etc.. Technically these could go in their own submodule, since they are relatively rarely used (IME, YMMV). But they are too few to really justify being in their own module, so maybe it's OK to group them in std.range.primitives along with array range primitives.

[quickfur]

Oh, and BTW, there's something else that needs to be cleaned up before the next release: https://issues.dlang.org/show_bug.cgi?id=13766

[9il]

@quickfur
InputRange and InputRangeObject are already in std.range.interfaces

[quickfur]

Yes I just saw that, that's why I edited my comment. :-)

[9il]

So I will rename current std.range.constraints to std.range.primitives.
For splitting can be other PR.

[quickfur]

Sounds good.

[dnadlinger]

@quickfur: Imho not including the array -> range adapters in the same module as the range traits is quite dangerous, because you need to import the former too so that generic code using isInputRange and others is actually correct. Sure, you can still mess things up by using selective imports, but there is little we can do about this.

Incidentally, this is also the reason why I think it's a mistake to keep the additional helpers like popFrontN in the same module as the core set of primitives. We want import std.range.primitives; to be used by default, so it should pull in as few names as possible.

[dnadlinger]

I agree that the further layout can be discussed in a separate PR, though.

[quickfur]

@klickverbot I agree with your points. So we should put array primitives and range traits together in std.range.primitives, and let popFrontN and other such helpers be placed elsewhere, say std.range.util or something like that. That can be sorted out later.

[MartinNowak]

Sounds like a plan.

[9il]

Renamed, see #2765