Re: [PATCH] Reference leak in `iprin1 ()'

[Ludovic Courtès]

Hi Neil,

Neil Jerram <address@hidden> writes:

> Why?  Does () have any symptoms that we should be concerned about?

Well, nothing to be really "concerned" about, mostly consistency.  In a
declaration, `()' has a different meaning than `(void)', but not in the
definition.  According to section 6.7.5.3 or ISO/IEC 9899:1999:

  10 The special case of an unnamed parameter of type void as the only
     item in the list specifies that the function has no parameters.

  [...]

  14 An identifier list declares only the identifiers of the parameters
     of the function. An empty list in a function declarator that is
     part of a definition of that function specifies that the function
     has no parameters. The empty list in a function declarator that is
     not part of a definition of that function specifies that no
     information about the number or types of the parameters is
     supplied.124)

Since, the declarations in `print.h' use `(void)', there's nothing
really serious here.

>> Below is an updated patch.  I modified `PUSH_REF ()' as well so that it
>> does PSTATE->TOP++ _after_ using the `PSTATE_STACK_SET ()' macro: this
>> is safer.
>
> Why is it safer?

Because of the side effects these macros may introduce, as described in
http://gcc.gnu.org/onlinedocs/gcc-4.0.2/cpp/Duplication-of-Side-Effects.html#Duplication-of-Side-Effects
 .

As far as `PUSH_REF ()' is concerned, without knowing all the
definitions of all the macros it uses, one can't be sure that
PSTATE->TOP won't be incremented more than once.

> My inclination is that it's usually safer to change
> less code, other things being equal.

Agreed.  But that must not prevent us from discussing existing code and
modifying it when we see fit, i.e. when there is improvement to gain.
;-)

Thanks,
Ludovic.