Re: svn commit: r1003986 [1/2] - in /subversion/trunk/subversion: libsvn_client/ libsvn_fs_base/ libsvn_fs_base/bdb/ libsvn_fs_fs/ libsvn_ra_local/ libsvn_ra_neon/ libsvn_ra_serf/ libsvn_ra_svn/ libsvn_repos/ libsvn_subr/ libsvn_wc/ mod_authz_svn/ mod_...

[Greg Hudson <gh...@MIT.EDU>]

On Mon, 2010-10-04 at 06:14 -0400, Julian Foad wrote:
> The NULL macro is intended for use as a pointer.

Only when statically cast to the appropriate pointer type.  This happens
automatically in many contexts, such as assignments or prototyped
function parameters.  But it does not happen automatically for variable
parameters of a stdarg function.

So apr_pstrcat(foo, bar, NULL) really is invalid C code.  It's not a
practical concern because common platforms use a single pointer
representation, but it's a fair warning for a compiler to give.

This message brought to you by Language Lawyers Inc.

Re: svn commit: r1003986 [1/2] - in /subversion/trunk/subversion: libsvn_client/ libsvn_fs_base/ libsvn_fs_base/bdb/ libsvn_fs_fs/ libsvn_ra_local/ libsvn_ra_neon/ libsvn_ra_serf/ libsvn_ra_svn/ libsvn_repos/ libsvn_subr/ libsvn_wc/ mod_authz_svn/ mod_...

[Greg Hudson <gh...@MIT.EDU>]

On Mon, 2010-10-04 at 12:06 -0400, Julian Foad wrote:
> The issue at hand is when NULL is defined as an unadorned '0' *and* is
> passed to a variadic function such as apr_pstrcat.  If that's not a
> practical concern, that must be because the size and representation of
> (int)0 is the same as (char *)0.  If that is true on all supported
> platforms, then omitting the casts is a valid option; otherwise, we need
> them.

I don't think you have it quite right.

If NULL is defined as, say, (void *)0, then the compiler might not warn,
but it's still not correct C to pass a void * to a variadic function and
then read it out as some other pointer type.  void * is only guaranteed
to work if it's cast (implicitly or implicitly) to the correct pointer
type.  We don't see this problem as run-time errors because there is
typically no runtime transformation between void * and other pointer
types.

If NULL is defined as 0, it would be easy to imagine a practical problem
where you pass a 32-bit 0 value to a variadic function and then read it
back out as a 64-bit pointer type.  We don't see this problem in
practice because NULL is typically defined as 0L rather than just 0, and
sizeof(long) typically matches the size of pointers.

In terms of code hygiene, passing an uncasted NULL to a variadic
function isn't any worse than using calloc() to initialize pointer
fields in structures.  But if your compiler is giving you grief about
it, the best thing to do is probably to get used to casting in that
situation.

Phillip wrote:
> In C++ the cast should be more common since a conforming NULL cannot
> have a cast but, in the free software world at least, GCC uses
> compiler magic to make plain NULL work as a pointer without a cast.

Unlike C, C++ doesn't allow implicit casts from void * to other pointer
types, so defining NULL as (void *)0 would be pretty inconvenient there.

Re: svn commit: r1003986 [1/2] - in /subversion/trunk/subversion: libsvn_client/ libsvn_fs_base/ libsvn_fs_base/bdb/ libsvn_fs_fs/ libsvn_ra_local/ libsvn_ra_neon/ libsvn_ra_serf/ libsvn_ra_svn/ libsvn_repos/ libsvn_subr/ libsvn_wc/ mod_authz_svn/ mod_...

[Julian Foad <ju...@wandisco.com>]

On Mon, 2010-10-04 at 11:14 -0400, Greg Hudson wrote:
> On Mon, 2010-10-04 at 06:14 -0400, Julian Foad wrote:
> > The NULL macro is intended for use as a pointer.
> 
> Only when statically cast to the appropriate pointer type.  This happens
> automatically in many contexts, such as assignments or prototyped
> function parameters.  But it does not happen automatically for variable
> parameters of a stdarg function.
> 
> So apr_pstrcat(foo, bar, NULL) really is invalid C code.

Ah.  In that case, I retract my request for reversion.

>   It's not a
> practical concern because common platforms use a single pointer
> representation, but it's a fair warning for a compiler to give.

The issue at hand is when NULL is defined as an unadorned '0' *and* is
passed to a variadic function such as apr_pstrcat.  If that's not a
practical concern, that must be because the size and representation of
(int)0 is the same as (char *)0.  If that is true on all supported
platforms, then omitting the casts is a valid option; otherwise, we need
them.

Experience suggests it has been fine on all supported platforms to date.

- Julian