You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2010/03/11 08:27:50 UTC
Re: svn commit: r921716 - /subversion/trunk/subversion/libsvn_subr/mergeinfo.c
On Thu, Mar 11, 2010 at 03:08, <ju...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_subr/mergeinfo.c Thu Mar 11 08:08:06 2010
>...
> @@ -1311,23 +1309,20 @@ svn_mergeinfo_intersect2(svn_mergeinfo_t
> for (hi = apr_hash_first(apr_hash_pool_get(mergeinfo1), mergeinfo1);
> hi; hi = apr_hash_next(hi))
> {
> - apr_array_header_t *rangelist;
> - const void *path;
> - void *val;
> - apr_hash_this(hi, &path, NULL, &val);
> -
> - rangelist = apr_hash_get(mergeinfo2, path, APR_HASH_KEY_STRING);
> - if (rangelist)
> - {
> - SVN_ERR(svn_rangelist_intersect(&rangelist,
> - (apr_array_header_t *) val,
> - rangelist, consider_ineheritance,
> - scratch_pool));
> - if (rangelist->nelts > 0)
> + const char *path = svn_apr_hash_index_key(hi);
> + apr_array_header_t *rangelist1 = svn_apr_hash_index_val(hi);
> + apr_array_header_t *rangelist2;
const?
> +
> + rangelist2 = apr_hash_get(mergeinfo2, path, APR_HASH_KEY_STRING);
You could use svn_apr_hash_index_klen() rather than APR_HASH_KEY_STRING.
>...
> @@ -1488,15 +1480,15 @@ svn_mergeinfo_dup(svn_mergeinfo_t mergei
> {
> svn_mergeinfo_t new_mergeinfo = apr_hash_make(pool);
> apr_hash_index_t *hi;
> - const void *path;
> - apr_ssize_t pathlen;
> - void *rangelist;
>
> for (hi = apr_hash_first(pool, mergeinfo); hi; hi = apr_hash_next(hi))
> {
> - apr_hash_this(hi, &path, &pathlen, &rangelist);
> + const char *path = svn_apr_hash_index_key(hi);
> + apr_ssize_t pathlen = svn_apr_hash_index_klen(hi);
> + apr_array_header_t *rangelist = svn_apr_hash_index_val(hi);
const?
> +
> apr_hash_set(new_mergeinfo, apr_pstrmemdup(pool, path, pathlen), pathlen,
> - svn_rangelist_dup((apr_array_header_t *) rangelist, pool));
> + svn_rangelist_dup(rangelist, pool));
> }
>
> return new_mergeinfo;
> @@ -1513,25 +1505,23 @@ svn_mergeinfo_inheritable2(svn_mergeinfo
> apr_pool_t *scratch_pool)
> {
> apr_hash_index_t *hi;
> - const void *key;
> - apr_ssize_t keylen;
> - void *rangelist;
> -
> svn_mergeinfo_t inheritable_mergeinfo = apr_hash_make(result_pool);
> +
> for (hi = apr_hash_first(scratch_pool, mergeinfo);
> hi;
> hi = apr_hash_next(hi))
> {
> + const char *key = svn_apr_hash_index_key(hi);
> + apr_ssize_t keylen = svn_apr_hash_index_klen(hi);
> + apr_array_header_t *rangelist = svn_apr_hash_index_val(hi);
const?
>...
> @@ -1632,14 +1622,8 @@ svn_mergeinfo__remove_empty_rangelists(s
> {
> for (hi = apr_hash_first(pool, mergeinfo); hi; hi = apr_hash_next(hi))
> {
> - const void *key;
> - void *value;
> - const char *path;
> - apr_array_header_t *rangelist;
> -
> - apr_hash_this(hi, &key, NULL, &value);
> - path = key;
> - rangelist = value;
> + const char *path = svn_apr_hash_index_key(hi);
> + apr_array_header_t *rangelist = svn_apr_hash_index_val(hi);
const?
>...
> @@ -1859,14 +1840,7 @@ svn_mergeinfo__get_range_endpoints(svn_r
>
> for (hi = apr_hash_first(pool, mergeinfo); hi; hi = apr_hash_next(hi))
> {
> - const void *key;
> - void *value;
> - const char *path;
> - apr_array_header_t *rangelist;
> -
> - apr_hash_this(hi, &key, NULL, &value);
> - path = key;
> - rangelist = value;
> + apr_array_header_t *rangelist = svn_apr_hash_index_val(hi);
const?
>...
> @@ -1950,14 +1920,8 @@ svn_mergeinfo__filter_mergeinfo_by_range
> hi;
> hi = apr_hash_next(hi))
> {
> - const void *key;
> - void *value;
> - const char *path;
> - apr_array_header_t *rangelist;
> -
> - apr_hash_this(hi, &key, NULL, &value);
> - path = key;
> - rangelist = value;
> + const char *path = svn_apr_hash_index_key(hi);
> + apr_array_header_t *rangelist = svn_apr_hash_index_val(hi);
const?
Cheers,
-g
Re: svn commit: r921716 -
/subversion/trunk/subversion/libsvn_subr/mergeinfo.c
Posted by Julian Foad <ju...@wandisco.com>.
I (Julian Foad) wrote:
> Yup, "const" would be better. But in order to do that we need to
> constify the various functions in this part of the API, such as
> svn_rangelist_dup().
>
> I'll do a big sweeping change, constifying all apr_array_header_t *'s
> that appear as inputs (i.e. logically const) in public and private APIs.
> (Note that we are allowed to constify public APIs - and have done so
> before - because that is a backward-compatible API change, and does not
> change the ABI.)
r922239.
- Julian
Re: svn commit: r921716 -
/subversion/trunk/subversion/libsvn_subr/mergeinfo.c
Posted by Julian Foad <ju...@wandisco.com>.
Greg Stein wrote:
> On Thu, Mar 11, 2010 at 03:08, <ju...@apache.org> wrote:
> >...
> > +++ subversion/trunk/subversion/libsvn_subr/mergeinfo.c Thu Mar 11 08:08:06 2010
> >...
> > @@ -1311,23 +1309,20 @@ svn_mergeinfo_intersect2(svn_mergeinfo_t
> > for (hi = apr_hash_first(apr_hash_pool_get(mergeinfo1), mergeinfo1);
> > hi; hi = apr_hash_next(hi))
> > {
> > - apr_array_header_t *rangelist;
> > - const void *path;
> > - void *val;
> > - apr_hash_this(hi, &path, NULL, &val);
> > -
> > - rangelist = apr_hash_get(mergeinfo2, path, APR_HASH_KEY_STRING);
> > - if (rangelist)
> > - {
> > - SVN_ERR(svn_rangelist_intersect(&rangelist,
> > - (apr_array_header_t *) val,
> > - rangelist, consider_ineheritance,
> > - scratch_pool));
> > - if (rangelist->nelts > 0)
> > + const char *path = svn_apr_hash_index_key(hi);
> > + apr_array_header_t *rangelist1 = svn_apr_hash_index_val(hi);
> > + apr_array_header_t *rangelist2;
>
> const?
(throughout the file)
Yup, "const" would be better. But in order to do that we need to
constify the various functions in this part of the API, such as
svn_rangelist_dup().
I'll do a big sweeping change, constifying all apr_array_header_t *'s
that appear as inputs (i.e. logically const) in public and private APIs.
(Note that we are allowed to constify public APIs - and have done so
before - because that is a backward-compatible API change, and does not
change the ABI.)
> > +
> > + rangelist2 = apr_hash_get(mergeinfo2, path, APR_HASH_KEY_STRING);
>
> You could use svn_apr_hash_index_klen() rather than APR_HASH_KEY_STRING.
Yup, we could do that in all sorts of places in another change.
- Julian