[PATCH hail] const-correctness tweaks

[PATCH hail] const-correctness tweaks

[Jim Meyering]

Make write_cb callback's buffer parameter const, like all write-like
functions.
Give a few "char *" parameters the "const" attribute.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---

It looks like most of hail's interfaces are const-correct,
but one stood out because it provokes a warning when I tried to
pass a const-correct write_cb function to hstor_get from iwhd:

    proxy.c:382: warning: passing argument 4 of 'hstor_get' from \
      incompatible pointer type
    /usr/include/hstor.h:173: note: expected \
      'size_t (*)(void *, size_t, size_t,  void *)' but argument is of type \
      'size_t (*)(const void *, size_t,  size_t,  void *)'

In case you feel comfortable fixing this, here's a patch:


 include/hstor.h |    4 ++--
 lib/hstor.c     |    5 +++--
 lib/hutil.c     |    2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/hstor.h b/include/hstor.h
index 8620d3b..b47387b 100644
--- a/include/hstor.h
+++ b/include/hstor.h
@@ -132,7 +132,7 @@ enum ReqACLC {
 /* hutil.c */
 extern char *hutil_time2str(char *buf, int len, time_t time);
 extern time_t hutil_str2time(const char *timestr);
-extern int hreq_hdr_push(struct http_req *req, char *key, char *val);
+extern int hreq_hdr_push(struct http_req *req, const char *key, const char *val);
 extern char *hreq_hdr(struct http_req *req, const char *key);
 extern void hreq_sign(struct http_req *req, const char *bucket, const char *key,
 	      char *b64hmac_out);
@@ -171,7 +171,7 @@ extern bool hstor_del_bucket(struct hstor_client *hstor, const char *name);
 extern struct hstor_blist *hstor_list_buckets(struct hstor_client *hstor);

 extern bool hstor_get(struct hstor_client *hstor, const char *bucket, const char *key,
-	     size_t (*write_cb)(void *, size_t, size_t, void *),
+	     size_t (*write_cb)(const void *, size_t, size_t, void *),
 	     void *user_data, bool want_headers);
 extern void *hstor_get_inline(struct hstor_client *hstor, const char *bucket,
 			    const char *key, bool want_headers, size_t *len);
diff --git a/lib/hstor.c b/lib/hstor.c
index d0d87c7..7f638ec 100644
--- a/lib/hstor.c
+++ b/lib/hstor.c
@@ -86,7 +86,8 @@ err_out:
 	return NULL;
 }

-static size_t all_data_cb(void *ptr, size_t size, size_t nmemb, void *user_data)
+static size_t all_data_cb(const void *ptr, size_t size, size_t nmemb,
+			  void *user_data)
 {
 	GByteArray *all_data = user_data;
 	int len = size * nmemb;
@@ -378,7 +379,7 @@ bool hstor_del_bucket(struct hstor_client *hstor, const char *name)
 }

 bool hstor_get(struct hstor_client *hstor, const char *bucket, const char *key,
-	     size_t (*write_cb)(void *, size_t, size_t, void *),
+	     size_t (*write_cb)(const void *, size_t, size_t, void *),
 	     void *user_data, bool want_headers)
 {
 	struct http_req req;
diff --git a/lib/hutil.c b/lib/hutil.c
index 7439d52..13a8d5e 100644
--- a/lib/hutil.c
+++ b/lib/hutil.c
@@ -131,7 +131,7 @@ static void cust_fin(struct custom_hdr_vec *cv)

 /*
  */
-int hreq_hdr_push(struct http_req *req, char *key, char *val)
+int hreq_hdr_push(struct http_req *req, const char *key, const char *val)
 {
 	struct http_hdr *hdr;

--
1.7.3.1.50.g1e633

Re: [PATCH hail] const-correctness tweaks

[Jeff Garzik]

On 10/06/2010 08:07 AM, Jim Meyering wrote:
>
> Make write_cb callback's buffer parameter const, like all write-like
> functions.
> Give a few "char *" parameters the "const" attribute.
>
> Signed-off-by: Jim Meyering<meyering@redhat.com>
> ---
>
> It looks like most of hail's interfaces are const-correct,
> but one stood out because it provokes a warning when I tried to
> pass a const-correct write_cb function to hstor_get from iwhd:
>
>      proxy.c:382: warning: passing argument 4 of 'hstor_get' from \
>        incompatible pointer type
>      /usr/include/hstor.h:173: note: expected \
>        'size_t (*)(void *, size_t, size_t,  void *)' but argument is of type \
>        'size_t (*)(const void *, size_t,  size_t,  void *)'
>
> In case you feel comfortable fixing this, here's a patch:
>
>
>   include/hstor.h |    4 ++--
>   lib/hstor.c     |    5 +++--
>   lib/hutil.c     |    2 +-
>   3 files changed, 6 insertions(+), 5 deletions(-)

This requires updating test/large-object.c in tabled, too.  Would you 
mind sending along that companion patch?

Re: [PATCH hail] const-correctness tweaks

[Jim Meyering]

Jeff Garzik wrote:
> On 10/06/2010 08:07 AM, Jim Meyering wrote:
...
>> It looks like most of hail's interfaces are const-correct,
>> but one stood out because it provokes a warning when I tried to
>> pass a const-correct write_cb function to hstor_get from iwhd:
...
>>   include/hstor.h |    4 ++--
>>   lib/hstor.c     |    5 +++--
>>   lib/hutil.c     |    2 +-
>>   3 files changed, 6 insertions(+), 5 deletions(-)
>
> This requires updating test/large-object.c in tabled, too.  Would you
> mind sending along that companion patch?

Sure.  Posting separately.
With only one use, the tendrils were relatively short.

Re: [PATCH hail] const-correctness tweaks

[Jeff Garzik]

On 10/06/2010 08:07 AM, Jim Meyering wrote:
>
> Make write_cb callback's buffer parameter const, like all write-like
> functions.
> Give a few "char *" parameters the "const" attribute.
>
> Signed-off-by: Jim Meyering<meyering@redhat.com>
> ---
>
> It looks like most of hail's interfaces are const-correct,
> but one stood out because it provokes a warning when I tried to
> pass a const-correct write_cb function to hstor_get from iwhd:
>
>      proxy.c:382: warning: passing argument 4 of 'hstor_get' from \
>        incompatible pointer type
>      /usr/include/hstor.h:173: note: expected \
>        'size_t (*)(void *, size_t, size_t,  void *)' but argument is of type \
>        'size_t (*)(const void *, size_t,  size_t,  void *)'
>
> In case you feel comfortable fixing this, here's a patch:


Sorry for not getting back to this; I had hoped to solve some additional 
problems that cropped up, but didn't have time.  So, to forestall 
further delay,

libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I.. -I../include -pthread 
-I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include 
-I/usr/include/libxml2 -O2 -Wall -Wshadow -g -MT hutil.lo -MD -MP -MF 
.deps/hutil.Tpo -c hutil.c -o hutil.o
hutil.c: In function ‘hreq_hdr_push’:
hutil.c:145: warning: assignment discards qualifiers from pointer target 
type
hutil.c:146: warning: assignment discards qualifiers from pointer target 
type

warnings appear after this patch.  When solving these warnings with 
'const' markers, it quickly becomes a bit of a rat's nest.

At a minimum, the write_cb callback signature must match libcurl's, 
which does not use 'const'.  I can see this makes sense from libcurl 
implementation's perspective, even if it does not really match the 
constness one expects from a foo-get function.

	Jeff

Re: [PATCH hail] const-correctness tweaks

[Jim Meyering]

Jeff Garzik wrote:

> On 10/06/2010 08:07 AM, Jim Meyering wrote:
>>
>> Make write_cb callback's buffer parameter const, like all write-like
>> functions.
>> Give a few "char *" parameters the "const" attribute.
>>
>> Signed-off-by: Jim Meyering<meyering@redhat.com>
>> ---
>>
>> It looks like most of hail's interfaces are const-correct,
>> but one stood out because it provokes a warning when I tried to
>> pass a const-correct write_cb function to hstor_get from iwhd:
>>
>>      proxy.c:382: warning: passing argument 4 of 'hstor_get' from \
>>        incompatible pointer type
>>      /usr/include/hstor.h:173: note: expected \
>>        'size_t (*)(void *, size_t, size_t,  void *)' but argument is of type \
>>        'size_t (*)(const void *, size_t,  size_t,  void *)'
>>
>> In case you feel comfortable fixing this, here's a patch:
>
>
> Sorry for not getting back to this; I had hoped to solve some
> additional problems that cropped up, but didn't have time.  So, to
> forestall further delay,
>
> libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I.. -I../include -pthread
> -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include
> -I/usr/include/libxml2 -O2 -Wall -Wshadow -g -MT hutil.lo -MD -MP -MF
> .deps/hutil.Tpo -c hutil.c -o hutil.o
> hutil.c: In function ‘hreq_hdr_push’:
> hutil.c:145: warning: assignment discards qualifiers from pointer
> target type
> hutil.c:146: warning: assignment discards qualifiers from pointer
> target type
>
> warnings appear after this patch.  When solving these warnings with
> const' markers, it quickly becomes a bit of a rat's nest.
>
> At a minimum, the write_cb callback signature must match libcurl's,
> which does not use 'const'.  I can see this makes sense from libcurl
> implementation's perspective, even if it does not really match the
> constness one expects from a foo-get function.

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.

diff --git a/lib/hutil.c b/lib/hutil.c
index 13a8d5e..b74460b 100644
--- a/lib/hutil.c
+++ b/lib/hutil.c
@@ -142,8 +142,8 @@ int hreq_hdr_push(struct http_req *req, const char *key, const char *val)
 		val++;

 	hdr = &req->hdr[req->n_hdr++];
-	hdr->key = key;
-	hdr->val = val;
+	hdr->key = (char *) key;
+	hdr->val = (char *) val;

 	return 0;
 }

Re: [PATCH hail] const-correctness tweaks

[Jeff Garzik]

On 10/20/2010 04:00 AM, Jim Meyering wrote:
> Jeff Garzik wrote:
>
>> On 10/06/2010 08:07 AM, Jim Meyering wrote:
>>>
>>> Make write_cb callback's buffer parameter const, like all write-like
>>> functions.
>>> Give a few "char *" parameters the "const" attribute.
>>>
>>> Signed-off-by: Jim Meyering<meyering@redhat.com>
>>> ---
>>>
>>> It looks like most of hail's interfaces are const-correct,
>>> but one stood out because it provokes a warning when I tried to
>>> pass a const-correct write_cb function to hstor_get from iwhd:
>>>
>>>       proxy.c:382: warning: passing argument 4 of 'hstor_get' from \
>>>         incompatible pointer type
>>>       /usr/include/hstor.h:173: note: expected \
>>>         'size_t (*)(void *, size_t, size_t,  void *)' but argument is of type \
>>>         'size_t (*)(const void *, size_t,  size_t,  void *)'
>>>
>>> In case you feel comfortable fixing this, here's a patch:
>>
>>
>> Sorry for not getting back to this; I had hoped to solve some
>> additional problems that cropped up, but didn't have time.  So, to
>> forestall further delay,
>>
>> libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I.. -I../include -pthread
>> -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include
>> -I/usr/include/libxml2 -O2 -Wall -Wshadow -g -MT hutil.lo -MD -MP -MF
>> .deps/hutil.Tpo -c hutil.c -o hutil.o
>> hutil.c: In function ‘hreq_hdr_push’:
>> hutil.c:145: warning: assignment discards qualifiers from pointer
>> target type
>> hutil.c:146: warning: assignment discards qualifiers from pointer
>> target type
>>
>> warnings appear after this patch.  When solving these warnings with
>> const' markers, it quickly becomes a bit of a rat's nest.
>>
>> At a minimum, the write_cb callback signature must match libcurl's,
>> which does not use 'const'.  I can see this makes sense from libcurl
>> implementation's perspective, even if it does not really match the
>> constness one expects from a foo-get function.
>
> 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?

	Jeff

Re: [PATCH hail] const-correctness tweaks

[Jim Meyering]

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.

Re: [PATCH hail] const-correctness tweaks

[Jeff Garzik]

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

Re: [PATCH hail] const-correctness tweaks

[Jeff Garzik]

On 10/22/2010 09:49 PM, Jeff Garzik wrote:
> On 10/20/2010 04:53 AM, Jim Meyering wrote:
>> 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

er, s/const/const-free/


> 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.

Re: [PATCH hail] const-correctness tweaks

[Jim Meyering]

Jeff Garzik wrote:
...
>> 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.

Thanks!