Обсуждение: Walker/mutator prototype.

I'm trying to change all the walkers and mutators to have a more
strict prototype.  I had to do this with lots of casts.

I don't really like the idea of having all those generic pointer
types (Node * and void *), but currently see no better way to deal
with it.

I attached the patch.


Kurt

Kurt Roeckx <Q@ping.be> writes:

> I'm trying to change all the walkers and mutators to have a more
> strict prototype.  I had to do this with lots of casts.
> 
> I don't really like the idea of having all those generic pointer
> types (Node * and void *), but currently see no better way to deal
> with it.

This code is incorrect. You have to declare the function prototype to match
the parameters that will actually be passed, not to match how they'll be used.

By casting the function pointers you're confusing the compiler into thinking
the variables are already the correct format and don't need to be cast.

The correct way to write this type of code is to prototype the functions with
void* or Node* or whatever variables will actually be passed, then immediately
assign the arguments to a local variable of the correct type.

Admittedly I doubt you'll actually run into any problems on any architecture
you're likely to see. But the behaviour is undefined in ANSI 89 C. 

As a side-point, personally I find the profusion of casts at every callpoint
to be far uglier, and also more error-prone than the single cast at the
beginning of each call-back.

-- 
greg

On Sat, Dec 13, 2003 at 10:24:23PM -0500, Greg Stark wrote:
> Kurt Roeckx <Q@ping.be> writes:
> 
> > I'm trying to change all the walkers and mutators to have a more
> > strict prototype.  I had to do this with lots of casts.
> > 
> > I don't really like the idea of having all those generic pointer
> > types (Node * and void *), but currently see no better way to deal
> > with it.
> 
> This code is incorrect. You have to declare the function prototype to match
> the parameters that will actually be passed, not to match how they'll be used.
> 
> By casting the function pointers you're confusing the compiler into thinking
> the variables are already the correct format and don't need to be cast.
> 
> The correct way to write this type of code is to prototype the functions with
> void* or Node* or whatever variables will actually be passed, then immediately
> assign the arguments to a local variable of the correct type.

I did start by changing all the context's to void *, but you'll
loose the real type that it gets called with, so the other calls
will not generate warnings anymore because of wrong type.  So I
just casted the function pointers to the right type.

Anyway, I'll change it so that the last argument is void *
everywhere.


Kurt

Kurt Roeckx <Q@ping.be> writes:

> I did start by changing all the context's to void *, but you'll
> loose the real type that it gets called with, so the other calls
> will not generate warnings anymore because of wrong type.  

But at least you'll get a warning if someone passes a non-pointer or an
incorrect number of arguments altogether.

> So I just casted the function pointers to the right type.

But that means you'll *never* get a warning. Even if someone passes a function
that's completely inappropriate. That seems worse than the disease. 

Plus it's simply wrong since the compiler might actually invoke the function
incorrectly. When you call the function you have to call it through a function
pointer with the same type as the prototype the function was defined with to
guarantee all the casts are performed and the proper calling convention is
followed.

-- 
greg

Kurt Roeckx <Q@ping.be> writes:
> I'm trying to change all the walkers and mutators to have a more
> strict prototype.  I had to do this with lots of casts.

Forget it ;-).  There's a reason why they use a loose prototype,
and it's exactly what you just found: the notational penalty of
being strict vastly outweighs any possible benefit.

Arguably, given the need to cast everything to Node * or void *,
the tighter prototypes are contributing zero additional error
checking anyway.
        regards, tom lane

Greg Stark <gsstark@mit.edu> writes:
> Kurt Roeckx <Q@ping.be> writes:
>> I did start by changing all the context's to void *, but you'll
>> loose the real type that it gets called with, so the other calls
>> will not generate warnings anymore because of wrong type.  

> But at least you'll get a warning if someone passes a non-pointer or an
> incorrect number of arguments altogether.

Note that in practice, the walker/mutator routines are not called from
random places, but by a *very* small number of macros used in clauses.c.
Thus, the probability that someone will introduce a bug into the call
sites is small, and the probability that they'd not discover it
instantly is even smaller.

Given that consideration, I don't see what the point is of trying to
tighten these prototypes.  ISTM it adds notational clutter for
essentially zero gain.
        regards, tom lane