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

[Yoann Vandoorselaere]

On Thu, 2005-05-19 at 10:47 +0100, Ian Abbott wrote:
> On 18/05/2005 19:29, Yoann Vandoorselaere wrote:
> > On Wed, 2005-05-18 at 16:16 +0100, Ian Abbott wrote:
> > 
> >>On 17/05/2005 10:20, Yoann Vandoorselaere wrote:
> >>
> >>>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 you show how the Hash_data_freer callback is called from 
> >>hash_insert, because I can't see it!
> > 
> > It is not. The point was that the interface allow the user to specify a
> > callback for freeing the data, and thus the pointer to theses data
> > should not be marked as const.
> 
> In that case, I agree with Jim that the current prototype declaration is 
> correct.  The function does not modify the data through the pointer, 
> hence the thing it is pointing to should be declared const within the 
> function.

This is a matter of opinion. Some people will think that the function
should not use const at all since the interface allow the user to
specify a callback for freeing the data. Other will not. There is no
"correct" answer.

Enough argument on both  side have already been given for that issue.
Jim is the module maintainer and he took the decision to leave the
interface as it is right now, so let's end up this discussion. 

> There is an unavoidable removal of the const qualifier because the 
> function returns a non-const qualified pointer to the const qulaified 
> data (similar to strstr).  
>
> That could be avoided by making the function 
> return a 'const void *', but that would be the wrong thing to do, as the 
> caller might want to modify the data and would have to cast away the 
> const qualifier itself to do that.
>
> Your workaround using a union would hide the warning about removal of 
> the qualifier.  Whether or not it is worth implementing is a matter of 
> opinion.  Presumably, the same sort of trick would have to be done in 
> gnulib's strstr implementation and similar functions too.

The discussion was about removing the constness from the function
prototype, not modifying the function so that they return 'void *'.

The workaround using an union was a hack for killing the warning, and I
don't consider this as a clean solution. As stated above, the constness
issue is a matter of taste, and to me these prototype should not use
const in the first place.

-- 
Yoann Vandoorselaere <address@hidden>