Philip Martin wrote:
> Julian Foad <julianfoad_at_btopenworld.com> writes:
>
> > 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().
Oops, it is a safe API when used with temp vars. I lost sight of that.
> It's standard C, it's even in the comp.lang.c FAQ:
>
> http://www.c-faq.com/ptrs/genericpp.html
Thanks for persisting, Philip. That helped to clarify it.
I got confused by the fact that we use temp vars in many instances but
explicit casts in other instances (see below). This made me think the
two approaches were equally correct. I think I understand now that:
- the temp vars are essential for a strictly correct solution;
- we already use temp vars and get a strictly correct solution in the
majority of cases;
- I was proposing (in my "simple" solution) doing away with the temp
vars everywhere;
- so that "solution" would have brought every instance down to the
lowest common denominator of correctness.
Whether the present code in which some instances are invalid is better
than having all instances consistently invalid is debatable, but I see
your point.
[...]
> > I am trying to solve the problem of source code verbosity. We currently
> > use verbose code (either temporary variables or explicit casts) at the
>
> Are we using explicit casts? I thought we always used temporary
> variables.
In a minority of instances, we are using explicit casts:
:grep "apr_hash_this(.*(" subversion/*/*.c subversion/*/*/*.c
subversion/libsvn_client/repos_diff.c:1429:
apr_hash_this(hi, &deleted_path, NULL, (void *)&dpn);
subversion/libsvn_ra/compat.c:811:
apr_hash_this(hi, (void *) &path, NULL, &val);
subversion/libsvn_repos/hooks.c:439:
apr_hash_this(hi, (void *)&token, NULL, &val);
subversion/libsvn_repos/log.c:1193:
apr_hash_this(hi, (void *) &rp->path, NULL,
subversion/libsvn_repos/rev_hunt.c:1229:
apr_hash_this(hi, (void *) &path, NULL, (void *) &rangelist);
subversion/svndumpfilter/main.c:541:
apr_hash_this(hi, (const void **) &key, NULL, &val);
subversion/svn/log-cmd.c:370:
apr_hash_this(hi, (void *) &path, NULL, &val);
subversion/svn/log-cmd.c:549:
apr_hash_this(hi, (void *)&property, NULL, (void *)&value);
subversion/mod_dav_svn/reports/log.c:119:
apr_hash_this(hi, (void *)&name, NULL, (void *)&value);
subversion/mod_dav_svn/reports/log.c:169:
apr_hash_this(hi, (void *) &path, NULL, &val);
> > 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.
>
> If you are replacing the temporary variables with casts you are making
> the code worse (strictly speaking you are making the code invalid).
Right, I think I get it now.
So, I'll go and refine the version that uses temp vars.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2366883
Received on 2009-07-01 01:45:45 CEST