Re: [PATCH hail] const-correctness tweaks

[Jeff Garzik <jeff@garzik.org>]

On 10/20/2010 04:53 AM, Jim Meyering wrote:
> Jeff Garzik wrote:
> ...
>>> Hi Jeff.
>>>
>>> Sorry I didn't notice that the first time.
>>> I built with ./autogen.sh&&   ./configure&&   make.
>>> It looks like you recommend -Wall -Wshadow.
>>>
>>> The two warnings above are the only ones I see with the patch,
>>> and they're easy to fix.  When storing const pointer params into
>>> a struct like that, I've found that it's best to cast away the "const",
>>> which really does reflect the semantics: by using "const" on the
>>> parameter, I view it as promising not to deref through the pointer
>>> *in that function*.  Since it's usually not reasonable to make
>>> the struct member "const" (as you saw, it propagates too far
>>> and often ends up being contradictory), the lesser evil of the cast
>>> is preferable here.
>>>
>>> If you're still game, the following incremental patch seems to be
>>> enough for me:  Let me know and I'll resubmit the full one.
>>
>> Well, my primary concern now originates from curl_easy_setopt(3)
>> documentation:
>>
>>         CURLOPT_WRITEFUNCTION
>>                Function pointer that  should  match  the  following
>> 	      prototype: size_t  function(  void  *ptr,  size_t  size,
>> 	      size_t nmemb, void *stream);
>>
>> hstor's callback is passed directly to libcurl, so we seem to be bound
>> by outside constraints, no?
>
> I compiled hail (with that patch) on F13 with -Wall -Wshadow
> with no warnings.  That curl_easy_setopt documentation seems to be
> overly strict, or perhaps out of date?.  When I compare with the
> code (curl/typecheck-gcc.h), I see all of the necessary "const" attributes:
>
> ----------------------------------------
> /* evaluates to true if expr is of type curl_write_callback or "similar" */
> #define _curl_is_write_cb(expr)                                               \
>    (_curl_is_read_cb(expr) ||                                            \
>     __builtin_types_compatible_p(__typeof__(expr), __typeof__(fwrite)) ||      \
>     __builtin_types_compatible_p(__typeof__(expr), curl_write_callback) ||     \
>     _curl_callback_compatible((expr), _curl_write_callback1) ||                \
>     _curl_callback_compatible((expr), _curl_write_callback2) ||                \
>     _curl_callback_compatible((expr), _curl_write_callback3) ||                \
>     _curl_callback_compatible((expr), _curl_write_callback4) ||                \
>     _curl_callback_compatible((expr), _curl_write_callback5) ||                \
>     _curl_callback_compatible((expr), _curl_write_callback6))
> typedef size_t (_curl_write_callback1)(const char *, size_t, size_t, void*);
> typedef size_t (_curl_write_callback2)(const char *, size_t, size_t,
>                                         const void*);
> typedef size_t (_curl_write_callback3)(const char *, size_t, size_t, FILE*);
> typedef size_t (_curl_write_callback4)(const void *, size_t, size_t, void*);
> typedef size_t (_curl_write_callback5)(const void *, size_t, size_t,
>                                         const void*);
> typedef size_t (_curl_write_callback6)(const void *, size_t, size_t, FILE*);
> ----------------------------------------
>
> But even if curl were requiring some suboptimal signature,
> it would be nice not to impose that on all projects that use hail.
> Are there older curl headers that do require the const-free signature?
> If there are and you want to support them, too, let me know -- maybe
> I can cook up an autoconf test to make things work there, with minimal
> impact.

Nah, I wouldn't worry about the const signature, it's probably just out 
of date documentation.  If users appear running old OS's or OS versions, 
we can tackle autoconf'ing on a piecemeal basis as needs arise.

Committed these patches of yours to hail.git and tabled.git.

	Jeff