Senthil,
Please find my comments below,
I am not sure about the value of change!. I think this is needed only
at the API level need not be at the usage level inside functions.
Even if we change all the occurance of 'apr_hash_t *' with
svn_mergeinfo_t we peek at the corresponding variable as pure hashes so
we don't gain anything by 'abstracting the types' alone without
abstracting its accessors.
If we love abstracting this data-type we need to provide its own
accessor routines i.e we should not blatantly use apr_hash_get/set.
> Index: subversion/libsvn_client/mergeinfo.c
> ===================================================================
> --- subversion/libsvn_client/mergeinfo.c (revision 29523)
> +++ subversion/libsvn_client/mergeinfo.c (working copy)
> @@ -781,7 +781,7 @@
> }
>
> svn_error_t *
> -svn_client__elide_mergeinfo_for_tree(apr_hash_t *children_with_mergeinfo,
> +svn_client__elide_mergeinfo_for_tree(svn_mergeinfo_t
> children_with_mergeinfo,
Here children_with_mergeinfo is *not* a mergeinfo though they both are
apr_hash_t, so this new type is not correct.
> svn_wc_adm_access_t *adm_access,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
Anyways this function does not seem to be used. I think we can remove it.
> Index: subversion/libsvn_client/mergeinfo.h
> ===================================================================
> --- subversion/libsvn_client/mergeinfo.h (revision 29523)
> +++ subversion/libsvn_client/mergeinfo.h (working copy)
> @@ -248,7 +248,7 @@
> /* A wrapper which calls svn_client__elide_mergeinfo() on each child
> in CHILDREN_WITH_MERGEINFO in depth-first. */
> svn_error_t *
> -svn_client__elide_mergeinfo_for_tree(apr_hash_t *children_with_mergeinfo,
> +svn_client__elide_mergeinfo_for_tree(svn_mergeinfo_t
> children_with_mergeinfo,
> svn_wc_adm_access_t *adm_access,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
Same as above.
> Index: subversion/bindings/swig/core.i
> ===================================================================
> --- subversion/bindings/swig/core.i (revision 29523)
> +++ subversion/bindings/swig/core.i (working copy)
> @@ -264,16 +264,16 @@
> /*
> -----------------------------------------------------------------------
> input mergeinfo hash
> */
> -%apply apr_hash_t *MERGEINFO {
> - apr_hash_t *mergefrom,
> - apr_hash_t *mergeto,
> - apr_hash_t *mergein1,
> - apr_hash_t *mergein2,
> - apr_hash_t *mergeinfo,
> - apr_hash_t *mergeinput,
> - apr_hash_t *eraser,
> - apr_hash_t *whiteboard,
> - apr_hash_t *changes
> +%apply svn_mergeinfo_t *MERGEINFO {
> + svn_mergeinfo_t *mergefrom,
> + svn_mergeinfo_t *mergeto,
> + svn_mergeinfo_t *mergein1,
> + svn_mergeinfo_t *mergein2,
> + svn_mergeinfo_t *mergeinfo,
> + svn_mergeinfo_t *mergeinput,
> + svn_mergeinfo_t *eraser,
I don't know much of bindings, but your change is 'type-wise totally
wrong in this file', you are replacing 'apr_hash_t *mergefrom' with
'svn_mergeinfo_t *mergefrom' which should have been 'svn_mergeinfo_t
mergefrom'.
> static svn_error_t *
> -svn_swig_mergeinfo_merge(apr_hash_t **mergeinfo_inout,
> - apr_hash_t *changes,
> +svn_swig_mergeinfo_merge(svn_mergeinfo_t **mergeinfo_inout,
> + svn_mergeinfo_t *changes,
> apr_pool_t *pool)
Type error!.
> {
> return svn_mergeinfo_merge(*mergeinfo_inout, changes, pool);
> }
>
> static svn_error_t *
> -svn_swig_mergeinfo_sort(apr_hash_t **mergeinfo_inout, apr_pool_t *pool)
> +svn_swig_mergeinfo_sort(svn_mergeinfo_t **mergeinfo_inout, apr_pool_t
> *pool)
Type error!.
> Index: subversion/libsvn_ra_serf/ra_serf.h
> ===================================================================
> --- subversion/libsvn_ra_serf/ra_serf.h (revision 29523)
> +++ subversion/libsvn_ra_serf/ra_serf.h (working copy)
> @@ -1189,7 +1189,7 @@
> apr_pool_t *pool);
>
> svn_error_t * svn_ra_serf__get_mergeinfo(svn_ra_session_t *ra_session,
> - apr_hash_t **mergeinfo,
> + svn_mergeinfo_t **mergeinfo,
Type error!.
> Index: subversion/tests/libsvn_client/client-test.c$
> ===================================================================$
> --- subversion/tests/libsvn_client/client-test.c^I(revision 29523)$
> +++ subversion/tests/libsvn_client/client-test.c^I(working copy)$
> @@ -70,7 +70,7 @@$
> catalog = apr_hash_make(iterpool);$
> for (item = elide_testcases[i]; item->path; item++)$
> {$
> - apr_hash_t *mergeinfo;$
> + svn_mergeinfo_t mergeinfo;$
> $
> SVN_ERR(svn_mergeinfo_parse(&mergeinfo,
> item->unparsed_mergeinfo, $
> iterpool));$
> @@ -81,7 +81,7 @@$
> $
> for (item = elide_testcases[i]; item->path; item++)$
> {$
> - apr_hash_t *mergeinfo = apr_hash_get(catalog, item->path, $
> + svn_mergeinfo_t *mergeinfo = apr_hash_get(catalog,
> item->path, $
Type error!.
Thanks
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-02-22 14:35:32 CET