Helper function for isInputRange to help with compile-time errors

[atilaneves]

As seen is Adam D. Ruppe's D Cookbook. This commit lets users assert that
their own types are input ranges and get meaninful compiler errors for
when their static assertions fail.

[ColonelThirtyTwo]

I don't think actually executing the range is a good idea; this test assumes that T.init creates a valid range (not true for any nontrivial range) and that it contains at least two elements (afaik calling popFront or front on an empty range is invalid).

[atilaneves]

My bad, I left out the negation.

[d-random-contributor]

It's supposed to be called from static assert, so !ctfe branch is never taken.
Without ctfe:

bool checkInputRange(R)()
{
  void checkCompilation(R r)
  {
    if (!r.empty) // can test for empty
    {
      auto h = r.front; // can get the front of the range
      r.popFront(); // can invoke popFront()
    }
  }
  return true;
}

Though should check if the (good) compiler eliminates the nested function.

[jmdavis]

This is an interesting idea, though I'm not sure that I like the idea of having isInputRange instantiate checkInputRange. In principle, it's a good idea, because it avoids duplicating the guts of isInputRange. However, one of the biggest problems with memory consumption and compilation times of template-heavy code is all of the templates that get compiled with template constraints - and isInputRange is quite possibly the most instantiated template that there is. Compiling std.algorithm's unit tests ends up creating millions of instantiations of templates, and if we did this with not only isInputRange but all of the similar eponymous templates, that's only going to increase the number of template instantiations and thus increase the memory consumption and compilation times. So, because of that problem, it might be better to just duplicate the guts of isInputRange inside of checkInputRange as annoying as that may be - though ideally we'd find ways to improve the compiler so that it wouldn't be as big a problem.

[atilaneves]

I guess I'll have to measure the impact first. It doesn't seem obvious to me that it's faster by duplicating code.

[jmdavis]

I guess I'll have to measure the impact first. It doesn't seem obvious to me that it's faster by duplicating code.

Well, it means instantiating fewer templates, but I suppose that the exact effect on speed would depend on exactly why instantiating the template is ultimately so expensive. e.g. if it's due to the list of instantiated templates that's maintained, then it could definitely be slower, whereas if it's due to the amount of code within the templates that's compiled, then it would be slower. Certainly, my initial reaction is that instantiating more templates is going to slow things down, particularly when you consider how many are already being instantiated, but you have a good point that we really need to measure it to see.

And from a maintenance perspective, it would definitely be better to not duplicate the code.

[WalterBright]

I don't understand. What's wrong with:

static assert(isInputRange!MyRange);

? (I do that myself for my own ranges.) How does checkInputRange do it better?

[PetarKirov]

@WalterBright The difference is that the current version of isInputRange does not provide error message saying what's wrong what your range candidate type. As you can see here (click compile to see the result),
static assert(isInputRange!MyRange); only says:
Error: static assert (isInputRange!(MyRange)) is false
and the checkInputRange version tells exactly what's wrong:
Error: no property 'empty' for type 'MyRange'
Error: template instance main.checkInputRange!(MyRange) error instantiating

[PetarKirov]

Related: forum.dlang.org: Interfaces, traits, concepts, and my idea for a DIP.

[atilaneves]

My doubt is that there's more work to be done at all. I don't immediately get why Instantiating checkInputRange (a full-blown template function) is more expensive than "instantiating" the lambda in the current version. It might cause template bloat, but I'm not even convinced that'd be the case (compared to now).

[atilaneves]

So, I copied out the code for the current version and the version in this PR, placed a version block around both of them to select them at compile-time then generated a file with 10k ranges each of them identical but for the name wrapping a slice and forwarding calls for each of the 3 input range primitives to the std.array versions. I then measured the time to compile the generated file for each version 10 times and took the minimum for each. I also used htop sorted by RSS to record memory usage and nm test.o | wc -l to see how many symbols were in the resulting object file. Results:

        time(s)    RSS(MB)    Symbols
old:    12.8       978        100047
new:    12.9       949        100047

As (I) expected, the compilation times are unaffected. Somewhat surprisingly, the memory usage actually goes down. The template bloat is identical.

[jmdavis]

isInputRange works just fine for statically asserting that a user-defined type is an input range. checkInputRange doesn't help with that at all. So, the docs really don't show how this is different from isInputRange. I think that the difference between the two should be made much clearer here. In particular, I think that it needs to be clear than isInputRange is used in template constraints (where you don't want to see error messages from inside of the traits you're using), whereas checkInputRange is used specifically on your user-defined types to report why they're failing isInputRange. And given the risk of confusion between isInputRange and checkInputRange, I'm inclined to think that we should find a way to do this such that checkInputRange isn't a function which returns bool and cannot possibly be used in a template constraint. I think that what you have going here is a great idea, but I think that we need to make sure that it's not going to cause increased confusion, and as long as it's possible to use it instead of isInputRange in a template constraint, I think that that's a real risk. They need to be distinct. And as it stands, they're not distinct enough in either the documentation or in how they can be used.

[atilaneves]

Right. The main problem here is that the unit tests fail because the code is actually executed (although it shouldn't) for interfaces. I need to find a better way of dealing with this.

[jmdavis]

Sorry, for not commenting back on this sooner. And clearly, I didn't look at the contents of the PR closely enough previously (see my recent comment). But if compilation times and memory are not adversely affected by this, then I'm all for it, assuming that the concerns in my previous comment are addressed.

[DmitryOlshansky]

This absolutely should get rid of stray commits first

[jmdavis]

This absolutely should get rid of stray commits first

Yeah. Those popped up a few days ago. Not sure what happened. But I'd like to see my concerns about checkInputRange being usable as a boolean be fixed, since the risk of confusion with isInputRange is high.

[atilaneves]

No idea what happened with those commits either, I'll clean it up but first I need to actually make it work.

[JackStouffer]

@atilaneves ping

[atilaneves]

I've nothing to add to this PR except to say that there's a DIP that makes it not matter.

[JackStouffer]

@atilaneves That's a shame, as I wanted something like this sooner rather than later.

Time to close then?

[wilzbach]

I've nothing to add to this PR except to say that there's a DIP that makes it not matter.

I am a huge fan of DIP83 (better assert error messages) and I remember also a couple of discussions on the mailing list about improving the error messages for constraints, thus probably the best is to bundle all the existing ideas & submit a great DIP.

Time to close then?

As this has stalled for more than one year, I will close it and I recommend to really go via the new DIP process or at least start a discussion on the NG as there seems to be no consensus on what way to go.