Philip Martin wrote:
> Julian Foad <julianfoad_at_btopenworld.com> writes:
> > Thanks for the response.
> >
> > The warnings I'm talking about (that I get from gcc if I use neither
> > casts nor temp vars with apr_hash_this()) are:
> >
> > warning: passing argument 2 [or 4] of 'apr_hash_this' from
> > incompatible pointer type
> >
> > I'm not sure if you're looking at the same issue.
>
> That's exactly the problem I described.
Maybe the difference is that you are thinking about the fact that the
types are incompatible (true, but not my concern), whereas I am thinking
about whether a warning is issued. A warning is issued if the conversion
is implicit but can be suppressed by making that conversion explicit. My
goal is to silence the warning without affecting the correctness (or, as
you point out, incorrectness) of the code. See below on why it doesn't
worsen the (in)correctness.
[...]
> The problem is that "svn_wc_entry_t*" isn't guaranteed to be big
> enough to hold the void* value that gets written by apr_hash_this. On
[...]
We're still not on the same page. That's not "the" problem, but "a"
problem. The problem you are describing is that apr_hash_this() is not
safe on mixed-pointer-size systems. I believe that is a problem with the
API of apr_hash_this().
I am not trying to solve that problem. I am happy to let that problem
continue to exist.
I am trying to solve the problem of source code verbosity. We currently
use verbose code (either temporary variables or explicit casts) at the
point of use of apr_hash_this() to achieve correct and warning-free
compilation (on a restricted but generally sufficient set of machine
architectures), whereas we could (and I want to) achieve the same with
terse code.
I believe that introducing intermediate type casts to the arguments to
apr_hash_this() silences the "incompatible pointer" warnings without
being any more unsafe than the API already is, and without worsening the
mixed-pointer-size problem that already exists.
I'm not clear whether you think I'm trying to (or need to) solve the
mixed-pointer-size problem, or whether you think my addition of an
intermediate cast introduces the mixed-pointer-size problem or makes it
worse, or something else.
Another clarification please?
> > The issue of whether (void **) is compatible with (&entry) is a separate
> > issue that has always existed with this APR API. Is that the
> > "type-punned" issue that you're talking about?
>
> Type-punning is a separate issue, I find it hard to work out whether
> it's a false alarm partly because the code has this other incompatible
> pointer problem.
>
> >
> >> > +#if 1
> >> > +#define APR_HASH_THIS(hi,key,klen,val) \
> >> > + apr_hash_this((hi), (void *)(key), (klen), (void *)(val))
> >>
> >> This is not valid for the reason above.
> >>
> >> > +#else
> >> > +#define APR_HASH_THIS(hi,key,klen,val) \
> >> > + do { \
> >> > + const void *__svn_key; \
> >> > + apr_ssize_t __svn_klen; \
> >> > + void *__svn_val; \
> >> > + apr_hash_this((hi), &__svn_key, &__svn_klen, &__svn_val); \
> >> > + if (key) *((const void **)key) = __svn_key; \
> >> > + if (klen) *((apr_ssize_t *)klen) = __svn_klen; \
> >> > + if (val) *((void **)val) = __svn_val; \
> >> > + } while (0)
> >> > +#endif
> >>
> >> That's not valid because the casts in the three assignment lines have
> >> the same problem. It should be OK without the casts.
> >
> > Here I agree that these casts are unsafe for systems on which pointers
> > are not all the same size. I added these casts only to be able to
> > compile this code without warnings when the "calling" code passes NULL
> > for key/klen/val. If I need to use an implementation like this one I
> > will look for a different way to achieve that.
>
> Three macros perhaps?
>
> APR_HASH_THIS_VAL(hi,val)
> APR_HASH_THIS_KEY_VAL(hi,key,val)
> APR_HASH_THIS_KEY_KLEN_VAL(hi,key,klen,val)
Yup, that will be a pretty good option if we can't agree on the simple
solution.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2366788
Received on 2009-06-30 19:58:54 CEST