assert(obj) is an atrocity

[Alex Rønne Petersen]

On 09-11-2011 01:57, Martin Nowak wrote:
> On Wed, 09 Nov 2011 01:21:47 +0100, Timon Gehr <timon.gehr at gmx.ch> wrote:
>
>> On 11/09/2011 01:18 AM, Martin Nowak wrote:
>>> On Tue, 08 Nov 2011 23:35:33 +0100, Alex Rønne Petersen
>>> <xtzgzorex at gmail.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> As the title suggests, I'm going to be rather blunt about this.
>>>> assert(obj) testing the invariant *without* doing a null check is
>>>> insane for the following reasons:
>>>>
>>>> 1) It is not what a user expects. It is *unintuitive*.
>>>> 2) assert(!obj) does an is-null check. assert(obj) is a completely
>>>> broken opposite of this.
>>>> 3) No AssertError is thrown, which is the entire point of the built-in
>>>> assert().
>>>> 4) The few added instructions for the null check hardly matter in a
>>>> *debug* build of all things.
>>>>
>>>> I don't mind assert(obj) testing the invariant of obj. In fact, that
>>>> very much makes sense. But please, please, *please* check the object
>>>> for null first. This is a random inconsistency in the language with no
>>>> other justification than "seg faults are convenient in a debugger". By
>>>> the same logic, we might as well not have array bounds checks.
>>>> However, the state of things is that array bounds checks are emitted
>>>> by default and users can disable them for e.g. a release build. I
>>>> don't see why this case is any different.
>>>>
>>>> - Alex
>>>
>>> It does check for null.
>>> Only it's a runtime support function (_d_invariant) and druntime is
>>> likely
>>> compiled without assertions. Are you really suggesting to add a null
>>> check before
>>> every method call?
>>>
>>> martin
>>
>> No, he is suggesting to add a null check for assert(objref);, a
>> construct that *looks* as if it was a null check, but does something
>> almost unrelated.
>
> Then just for reference, apply this patch to druntime and you'll get an
> assertion instead.
>
> diff --git a/src/rt/invariant.d b/src/rt/invariant.d
> index 71337f1..bc5e53a 100644
> --- a/src/rt/invariant.d
> +++ b/src/rt/invariant.d
> @@ -16,13 +16,16 @@
> /**
> *
> */
> +import core.exception;
> +
> void _d_invariant(Object o)
> { ClassInfo c;
>
> //printf("__d_invariant(%p)\n", o);
>
> // BUG: needs to be filename/line of caller, not library routine
> - assert(o !is null); // just do null check, not invariant check
> + if (o is null)
> + throw new AssertError("_d_invariant called with null reference.");
>
> c = o.classinfo;
> do

But as that BUG note states, it still isn't sufficient/consistent. :) 
Thanks for the patch though!

- Alex