const-correctness tweaks

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<[email protected]>

---

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(-)

On 10/06/2010 08:07 AM, Jim Meyering wrote:

Quoted 3 lines

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:

Quoted 4 lines

  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?

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<[email protected]>

---

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:

Quoted 12 lines · 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<[email protected]>

---

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:

Quoted 7 lines

     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

(quoted diff) +0 -2

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

Quoted 36 lines · Jeff Garzik

Quoted 12 lines · 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<[email protected]>

---

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:

Quoted 7 lines

      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

(quoted diff) +0 -2

-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

Quoted 7 lines

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.

Quoted 16 lines

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.

Quoted 2 lines

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

Quoted 22 lines

Quoted 16 lines

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.

Quoted 2 lines

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:

Quoted 7 lines

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.

Quoted 6 lines · Jim Meyering

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.

Quoted 6 lines

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.