Re: truth of %nil

[Mark H Weaver]

I would like to argue that the definitions of scm_is_false,
scm_is_true, and scm_is_null should indeed be changed to test for
%nil.

Do a grep-find in the tree for uses of these macros.  I think you'll
find that the majority of places where they are used should also be
checking for %nil, but they are not.

The only times when we can safely avoid testing for %nil is when we
know *statically* that the value being tested was not created by elisp
code.  Of course, in scheme, it is extremely rare that we can know
this statically.  Even bindings like `and', `or', and `not' could in
principle be bound to elisp functions.

More importantly, scm_is_false, scm_is_true, and scm_is_null in code
outside of guile's source tree should almost always be checking for
%nil, unless they know statically that their own code created the
value in question (because they shouldn't make assumptions about what
libguile's code will do in the future), which again is very rare.

Right now, there are scores of bugs in guile's tree that will only
show up sporadically for those doing heavy mixing of elisp and scheme,
because most code written in C (almost everything except for the
evaluator itself) is failing to check for %nil even though it should.

Do a quick grep for uses of scm_is_null in the C code for srfi-1, for
just one example.

The default should be to test for %nil.  If, in a particular use, it
can be proved statically that the value was not created by an elisp
function (which we can almost never prove), then that is a case where
we can use some faster test.  But someone will have to think about
each of these cases individually anyway, so it makes sense that these
faster tests should be named something different than the old names,
and preferably with a longer name, calling attention to the fact that
it is a potential source of bugs -- because even if at some point a
tested value can be proved to never be %nil, this might very well
change later, thus creating a new rarely-triggered bug in old code.

Maybe names something like this:

scm_is_false_xxx_assume_never_nil
scm_is_true_xxx_assume_never_nil
scm_if_null_xxx_assume_never_nil

One category of place where these could be used is code dealing with
data structures created internally by the evaluator -- though I'm not
very familiar with guile's internals, so I don't know how common these
data structures are, if indeed they exist at all.

Best regards,

   Mark


On Wed, Jul 01, 2009 at 10:54:50PM +0100, Neil Jerram wrote:
> OK, I see.  The point is that VM ops like br-if use SCM_FALSEP (which
> is equivalent to scm_is_false), and hence you're wondering if it would
> be easier to change the definition of scm_is_false, than to modify
> those ops to say (SCM_FALSEP (x) || SCM_NILP (x)).
> 
> I think the balance of arguments is clearly against doing that:
> 
> - There are lots of places that use scm_is_false where there is no
>   need to allow for the value being tested being %nil.  Changing
>   scm_is_false would be a performance hit for those places.
> 
> - There are only a handful of places (I think) that you need to change
>   to get %nil-falseness in the VM.
> 
> - There is a similar number of places which already implement
>   %nil-falseness in the interpreter by using (scm_is_false (x) ||
>   SCM_NILP (x)), and these would logically have to be changed if you
>   made your proposed change.
> 
> - It would be an incompatible API change.
> 
> So please just change the relevant places in the VM to say
> (scm_is_false (x) || SCM_NILP (x)) instead.
> 
> Regards,
>         Neil
> 
> 
>