While I was debugging std.range and std.algorithm I noticed that there's tons of inconsistency about when enforce() vs. assert() is used for checking input to range primitives.  I think this needs to be made consistent, and there are good arguments for both.

enforce() is arguably the way to go because we're dealing with an external API, not internal consistency checks.  According to the guidelines in TDPL input external to an API should be checked even in release mode.

On the other hand, assert() is arguably the way to go because ranges are supposed to be a very efficient abstraction.  Having enforce() in range primitives that look like they should be very fast, inlined functions is just bad practice.  For example, some benchmarks I did awhile back showed that the use of enforce() and its effect on inlining can make using ranges slower than using opApply.  Furthermore, if you're stacking several higher-order ranges, i.e. filter!"a > 0"(map!"a * a"(retro(someArray))) the checks may be redundant.  Lastly, since arrays are the canonical range, omitting release mode checks in library-defined ranges would make them consistent with arrays.

Generally when I was debugging I tried to be locally consistent, i.e. use whatever was used elsewhere in the same range.  This meant being globally inconsistent.  Now that I've thought about it, my vote is for using assert() consistently for range primitives.  If ranges are filled with tons of safety checks that have significant performance costs and can't be disabled even in release mode, they will have failed in their goal of providing a lightweight, efficient abstraction.  enforce() can be used for things that are intended to be called much less frequently, like constructors.  Comments?

The ideal situation would be to use asserts in all cases where it's going to be bugs in the program rather than bad user input and that they go away in release mode. The problem is the fact that Phobos is compiled as an external library rather than being compiled in with your code when you compile. And since there's no way (at least no standard way) to have a release and debug version of the Phobos libraries, with the release version being used in release mode and the debug version being used in debug mode, that pretty much means that you _always_ compile your code with the release version of Phobos. This makes assert _useless_ for users of the library. If you want checks beyond within Phobos code, you _have_ to use enforce(), and since enforce has issues with inlining and it doesn't go away in release mode, it's like forcing a portion of Phobos into debug mode.

_Ideally_, we would find some sort of standard way to make Phobos used in debug mode when compiling in debug mode and release mode when compiling in release mode. That way, asserts could be used in a lot of these cases since a lot of them really shouldn't be checked in release mode. Personally, I _really_ don't like the fact that the libraries aren't necessarily in the same mode as your code, though I'm not sure that there's a good way to deal with that other than having some standard naming scheme for debug vs release libraries which Phobos uses and dmd understands so that you get a debug version when compiling your code in debug mode, and you get a release version when compiling in release mode. Barring that however...

You'd almost have to take it on a case by case basis. In some cases, you're going to need checks and it would be bad not to have them, while in others, it would just cost too much to leave them in. It's a tough call. If anything though, I think that it's a prime example of needing to find a better way to deal with debug vs release and libraries. If we had a standard way to have both available, then dmd could use both with user code, and then we can use asserts like they should be used. As it stands though, we're probably going to have to use enforce() where we _need_ checks for safety and asserts otherwise. But since using asserts is almost pointless.... Bleh. It's just ugly.

- Jonathan M Davis

A valid criticism in general.  However, all of the ranges in question are templates, so each instantiation is compiled in whatever mode your program is compiled in.

On 8/18/2010 7:08 PM, Jonathan M Davis wrote:
> The ideal situation would be to use asserts in all cases where it's going to be bugs in the program rather than bad user input and that they go away in release mode. The problem is the fact that Phobos is compiled as an external library rather than being compiled in with your code when you compile. And since there's no way (at least no standard way) to have a release and debug version of the Phobos libraries, with the release version being used in release mode and the debug version being used in debug mode, that pretty much means that you _always_ compile your code with the release version of Phobos. This makes assert _useless_ for users of the library. If you want checks beyond within Phobos code, you _have_ to use enforce(), and since enforce has issues with inlining and it doesn't go away in release mode, it's like forcing a portion of Phobos into debug mode.
>
> _Ideally_, we would find some sort of standard way to make Phobos used in debug mode when compiling in debug mode and release mode when compiling in release mode. That way, asserts could be used in a lot of these cases since a lot of them really shouldn't be checked in release mode. Personally, I _really_ don't like the fact that the libraries aren't necessarily in the same mode as your code, though I'm not sure that there's a good way to deal with that other than having some standard naming scheme for debug vs release libraries which Phobos uses and dmd understands so that you get a debug version when compiling your code in debug mode, and you get a release version when compiling in release mode. Barring that however...
>
> You'd almost have to take it on a case by case basis. In some cases, you're going to need checks and it would be bad not to have them, while in others, it would just cost too much to leave them in. It's a tough call. If anything though, I think that it's a prime example of needing to find a better way to deal with debug vs release and libraries. If we had a standard way to have both available, then dmd could use both with user code, and then we can use asserts like they should be used. As it stands though, we're probably going to have to use enforce() where we _need_ checks for safety and asserts otherwise. But since using asserts is almost pointless.... Bleh. It's just ugly.
>
> - Jonathan M Davis
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos
>
> 

Le 2010-08-18 ? 19:08, Jonathan M Davis a ?crit :

> The ideal situation would be to use asserts in all cases where it's going to be bugs in the program rather than bad user input and that they go away in release mode.

I'd say the ideal situation would be to have the 'in' contracts checked on the caller's side. That way a program compiled in debug mode would still check the input it sends to Phobos, and whether Phobos was compiled in debug or release mode would only affect its internal checks, not the checks about its inputs.

-- 
Michel Fortin
michel.fortin at michelf.com
http://michelf.com/

On Wednesday, August 18, 2010 16:36:07 David Simcha wrote:
> A valid criticism in general.  However, all of the ranges in question are templates, so each instantiation is compiled in whatever mode your program is compiled in.

Ah good point. I missed that. That being the case, I'd probably lean towards assertions. Ranges need to be high performance. They should have very clear error messages with them rather than be bare assertions (so hopefully the programmer will realize that it's _their_ bug and not Phobos'), but still assertions. I think that ranges are too performance critical for enforce() if assertions are actually useable (and as you point out, since they're templates, assertions will be useable). So, I'd vote assertions.

- Jonathan M Davis

On Wednesday, August 18, 2010 17:09:59 Michel Fortin wrote:
> Le 2010-08-18 ? 19:08, Jonathan M Davis a ?crit :
> > The ideal situation would be to use asserts in all cases where it's going to be bugs in the program rather than bad user input and that they go away in release mode.
> 
> I'd say the ideal situation would be to have the 'in' contracts checked on the caller's side. That way a program compiled in debug mode would still check the input it sends to Phobos, and whether Phobos was compiled in debug or release mode would only affect its internal checks, not the checks about its inputs.

That would be nice. I doubt that that could be done very reasonably, if at all. But it would make good sense. If you could do it, there would be code bloat in the debug version, but it would go away in release, so it shouldn't be a big deal. A great idea, but I rather doubt it'll happen. But as you said, it would be the ideal situation.

- Jonathan M Davis

On 19 August 2010 02:27, Jonathan M Davis <jmdavisprog at gmail.com> wrote:
> On Wednesday, August 18, 2010 17:09:59 Michel Fortin wrote:
>> Le 2010-08-18 ? 19:08, Jonathan M Davis a ?crit :
>> > The ideal situation would be to use asserts in all cases where it's going to be bugs in the program rather than bad user input and that they go away in release mode.
>>
>> I'd say the ideal situation would be to have the 'in' contracts checked on the caller's side. That way a program compiled in debug mode would still check the input it sends to Phobos, and whether Phobos was compiled in debug or release mode would only affect its internal checks, not the checks about its inputs.
>
> That would be nice. I doubt that that could be done very reasonably, if at all. But it would make good sense. If you could do it, there would be code bloat in the debug version, but it would go away in release, so it shouldn't be a big deal. A great idea, but I rather doubt it'll happen. But as you said, it would be the ideal situation.

I'm not certain that the current scheme works correctly. There's an
extremely nasty bug:
3602 ICE(tocsym.c) compiling a class, if its super class has preconditions
as a result of in contracts being a nested function.

On Wed, 2010-08-18 at 20:09 -0400, Michel Fortin wrote:
> Le 2010-08-18 ? 19:08, Jonathan M Davis a ?crit :
> 
> > The ideal situation would be to use asserts in all cases where it's going to be bugs in the program rather than bad user input and that they go away in release mode.
> 
> I'd say the ideal situation would be to have the 'in' contracts checked on the caller's side. That way a program compiled in debug mode would still check the input it sends to Phobos, and whether Phobos was compiled in debug or release mode would only affect its internal checks, not the checks about its inputs.

I wonder if it would be feasible to have two versions of the Phobos lib, libphobos2.a and libphobos2-debug.a, distributed with DMD.  The former would be linked in when you compile with -release, otherwise the latter would be used.  The Phobos makefile already builds both versions, so it would just be a small compiler change.

As for the original question, I believe assert() is the way to go with
the range primitives.  The documentation for std.range clearly states
that front() and popFront() should only be called when empty() has, or
would have, returned false, which means it should be up to the user to
make sure this is always the case.

The way things are now, with many (most?) implementations of front() and
popFront() starting with enforce(!empty), empty() is called *three*
times for each iteration of your average foreach loop.

-Lars

That would be the -defaultlib and -debuglib options if I'm not misstaken.

On 19 aug 2010, at 01:08, Jonathan M Davis <jmdavisprog at gmail.com> wrote:

> The ideal situation would be to use asserts in all cases where it's going to be bugs in the program rather than bad user input and that they go away in release mode. The problem is the fact that Phobos is compiled as an external library rather than being compiled in with your code when you compile. And since there's no way (at least no standard way) to have a release and debug version of the Phobos libraries, with the release version being used in release mode and the debug version being used in debug mode, that pretty much means that you _always_ compile your code with the release version of Phobos. This makes assert _useless_ for users of the library. If you want checks beyond within Phobos code, you _have_ to use enforce(), and since enforce has issues with inlining and it doesn't go away in release mode, it's like forcing a portion of Phobos into debug mode.
> 
> _Ideally_, we would find some sort of standard way to make Phobos used in debug mode when compiling in debug mode and release mode when compiling in release mode. That way, asserts could be used in a lot of these cases since a lot of them really shouldn't be checked in release mode. Personally, I _really_ don't like the fact that the libraries aren't necessarily in the same mode as your code, though I'm not sure that there's a good way to deal with that other than having some standard naming scheme for debug vs release libraries which Phobos uses and dmd understands so that you get a debug version when compiling your code in debug mode, and you get a release version when compiling in release mode. Barring that however...
> 
> You'd almost have to take it on a case by case basis. In some cases, you're going to need checks and it would be bad not to have them, while in others, it would just cost too much to leave them in. It's a tough call. If anything though, I think that it's a prime example of needing to find a better way to deal with debug vs release and libraries. If we had a standard way to have both available, then dmd could use both with user code, and then we can use asserts like they should be used. As it stands though, we're probably going to have to use enforce() where we _need_ checks for safety and asserts otherwise. But since using asserts is almost pointless.... Bleh. It's just ugly.
> 
> - Jonathan M Davis
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos

Jacob <doob at me.com> wrote:

> That would be the -defaultlib and -debuglib options if I'm not misstaken.

Not quite. -debuglib is only chosen if used with -g. Also, unless sc.ini/dmd.conf gets -debuglib=phobos-debug added to DFLAGS, it's not gonna matter all that much, as it won't be standard.

-- 
Simen