[bug-gnulib] Re: [PATCH]: fix warning in the hash module

[Yoann Vandoorselaere]

On Tue, 2005-05-17 at 08:59 +0200, Jim Meyering wrote: 
> Yoann Vandoorselaere <address@hidden> wrote:
> > On Mon, 2005-05-16 at 15:20 +0200, Jim Meyering wrote:
> >> Yoann Vandoorselaere <address@hidden> wrote:
> >> > This patch fix constness warning in the GnuLib hash module.
> >>
> >> I'm all for avoiding warnings, but not when it detracts from what I
> >> think of as the correctness of an interface, as it would in this case.
> >>
> >> Those `const void *entry' parameters constitute a promise
> >> by the hash module that it will not try to write through
> >> those pointers.  Removing `const' here might make users of
> >> these functions wonder whether the functions write through
> >> the void pointers.
> >
> > Since the interface let the user provide a function for deleting data
> > (which thus does not use const), you might then as well workaround this
> > issue using:
> >
> > union {
> >     const void *val_ro;
> >     void *val_rw;
> > } user_data;
> >
> > And use the later in order to provide the deletion callback with a not
> > const pointer.
> > Proof of concept patch attached.
> 
> I'll try to make it clearer this time.  The promise is that the
> named functions and their callees will not dereference through
> the const pointers.  Those `const' attributes say nothing about
> what other functions (like the user-supplied deletion callback)
> will do to the underlying data.
> 
> This is similar to using a const pointer to iterate through
> non-const data because you're performing a search or doing some
> other read-only job.  The added (some might say `unnecessary')
> const makes the code a little more readable and might also give
> the compiler clues to let it generate better code.

I actually wonder if you looked at the second patch, since the point is
that it make no modification whatsoever to your original function
prototype, when here you appear to think that it does.  The promise you
are talking about is respected. 

Now, from my point of view this last patch is a workaround and the basic
API is not correct: You should not use const, it is not technically
correct as the user might free the data from the hash interface
callback.


> Thanks for the patch, but IMHO such a change is not necessary
> and would make hash.c (and its interfaces) less readable.

The interface won't be less readable because it is not modified in this
version of the patch. 

However, I still consider this version of the patch to be a hack, when
the real bug here is that you should not use const since you permit the
user to free() the data from the interface callback. The interface
should not use const at all.


> Can't you just turn off that particular warning when compiling hash.c?

I'm providing a patch since from my point of view, the culprit is the
code. Of course I could turn off the warning, but that's not how I plan
to fix it ;-) 

-- 
Yoann Vandoorselaere <address@hidden>