[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: PATCH: Stop defining identifiers in APR's name space

From: <kfogel_at_collab.net>
Date: 2003-12-19 18:30:17 CET

Julian Foad <julianfoad@btopenworld.com> writes:
> I believe this was mentioned as something that is needed before 1.0.
>
> Should I commit it?

I think it's a good idea to get it on trunk, at the very least. Not
sure what will happen to it in the 1.0 voting, but I must admit it
stands a reasonable chance.

Heh, I feel like Antonin Scalia in Bush v. Gore: "...the issuance of
the stay suggests that a majority of the Court, while not deciding the
issues presented, believe that the petitioner has a substantial
probability of success." :-)

Er, anyway. Some comments about the change itself:

> Stop defining Subversion identifiers in APR's name space.
>
> * subversion/include/svn_sorts.h
> * subversion/libsvn_subr/sorts.c
> (apr_hash_sorted_keys): Rename to "svn_sort_hash".
> (apr_array_prepend): Delete, as it was unused.
>
> * subversion/include/svn_types.h
> (apr_atoui64): Rename this macro to "svn_atoui64".
>
> * subversion/libsvn_fs/bdb/dbt.c
> (apr_free_cleanup): Rename this static function to "free_cleanup".
> (svn_fs__track_dbt): Adjust accordingly.
>
> * subversion/libsvn_subr/io.c
> (apr_dir_is_empty): Rename this static function to "dir_is_empty".
> (svn_io_dir_empty): Adjust accordingly.

Very minor nit: the usual style is to put the new name in parens, as
in

   (svn_sort_hash): New name for apr_hash_sorted_keys.

But also, Greg Stein made an interesting suggestion, quoted in the
issue (#1644):

> If we're pretty sure these will get into APR, we could name them
> svn__apr_foo and SVN__APR_FOO, make sure there are no doxygen
> comments for them, and note in the header files that they're for
> internal use only. Then it should be reasonably safe to remove
> them from the libraries in the future.
>
> If we think some of them won't make it into APR, then we should
> pick names we're comfortable with supporting.

I'm not sure, right now, which of these are likely to make it into
APR, but its seems prudent to start out by using auto-deprecated
names. We can always promote them to real names in a later release
once we know what's happened in APR.

To that end, the doc strings for these functions should also say that
these names should not be depended on, and should probably point to
the issue for further detail.

Regarding svn_sort_hash(), do we want to also rename svn_item_t to
match?

-Karl

> Index: subversion/include/svn_sorts.h
> ===================================================================
> --- subversion/include/svn_sorts.h (revision 8032)
> +++ subversion/include/svn_sorts.h (working copy)
> @@ -56,8 +56,7 @@
> * array, do this:
> *
> *<pre> apr_array_header_t *hdr;
> - * hdr = apr_hash_sorted_keys (hsh, @c svn_sort_compare_items_as_paths,
> - * pool);</pre>
> + * hdr = svn_sort_hash (hsh, @c svn_sort_compare_items_as_paths, pool);</pre>
> *
> * The key strings must be null-terminated, even though klen does not
> * include the terminator.
> @@ -77,7 +76,6 @@
> int svn_sort_compare_revisions (const void *a, const void *b);
>
>
> -#ifndef apr_hash_sorted_keys
> /** Sort @a ht according to its keys, return an @c apr_array_header_t
> * containing @c svn_item_t structures holding those keys and values
> * (i.e. for each @c svn_item_t @a item in the returned array, @a item->key
> @@ -94,25 +92,11 @@
> * This function and the @a svn_item_t should go over to APR. Got a Round Tuit?
> */
> apr_array_header_t *
> -apr_hash_sorted_keys (apr_hash_t *ht,
> - int (*comparison_func) (const svn_item_t *,
> - const svn_item_t *),
> - apr_pool_t *pool);
> -#endif /* apr_hash_sorted_keys */
> +svn_sort_hash (apr_hash_t *ht,
> + int (*comparison_func) (const svn_item_t *,
> + const svn_item_t *),
> + apr_pool_t *pool);
>
> -#ifndef apr_array_prepend
> -/** Return a pointer to an empty slot at the head of array @a arr.
> - *
> - * Note that if your application does not strictly need the new
> - * element to be at the head of the array, consider using the much
> - * more efficient function, apr_array_push() (which will add your new
> - * element at the tail of the array).
> - *
> - * NOTE:
> - * This function should go over to APR.
> - */
> -void *apr_array_prepend (apr_array_header_t *arr);
> -#endif /* apr_array_prepend */
>
> #ifdef __cplusplus
> }
> Index: subversion/include/svn_types.h
> ===================================================================
> --- subversion/include/svn_types.h (revision 8032)
> +++ subversion/include/svn_types.h (working copy)
> @@ -123,7 +123,7 @@
> #define SVN_FILESIZE_T_FMT APR_UINT64_T_FMT
>
> /* FIXME: Have to fiddle with APR to define this function */
> -#define apr_atoui64(X) ((apr_uint64_t) apr_atoi64(X))
> +#define svn_atoui64(X) ((apr_uint64_t) apr_atoi64(X))
>
>
> /** YABT: Yet Another Boolean Type */
> Index: subversion/libsvn_fs/bdb/dbt.c
> ===================================================================
> --- subversion/libsvn_fs/bdb/dbt.c (revision 8032)
> +++ subversion/libsvn_fs/bdb/dbt.c (working copy)
> @@ -72,7 +72,7 @@
> /* An APR pool cleanup function that simply applies `free' to its
> argument. */
> static apr_status_t
> -apr_free_cleanup (void *arg)
> +free_cleanup (void *arg)
> {
> free (arg);
>
> @@ -84,7 +84,7 @@
> svn_fs__track_dbt (DBT *dbt, apr_pool_t *pool)
> {
> if (dbt->data)
> - apr_pool_cleanup_register (pool, dbt->data, apr_free_cleanup,
> + apr_pool_cleanup_register (pool, dbt->data, free_cleanup,
> apr_pool_cleanup_null);
>
> return dbt;
> Index: subversion/libsvn_fs/util/fs_skels.c
> ===================================================================
> --- subversion/libsvn_fs/util/fs_skels.c (revision 8032)
> +++ subversion/libsvn_fs/util/fs_skels.c (working copy)
> @@ -555,7 +555,7 @@
> window_skel->children->next->next->data,
> window_skel->children->next->next->len);
> chunk->offset =
> - apr_atoui64 (apr_pstrmemdup (pool,
> + svn_atoui64 (apr_pstrmemdup (pool,
> chunk_skel->children->data,
> chunk_skel->children->len));
>
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 8032)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -2022,7 +2022,7 @@
> * then return the appropriate apr status code.
> */
> static apr_status_t
> -apr_dir_is_empty (const char *dir, apr_pool_t *pool)
> +dir_is_empty (const char *dir, apr_pool_t *pool)
> {
> apr_status_t apr_err;
> apr_dir_t *dir_handle;
> @@ -2079,7 +2079,7 @@
>
> SVN_ERR (svn_path_cstring_from_utf8 (&path_apr, path, pool));
>
> - status = apr_dir_is_empty (path_apr, pool);
> + status = dir_is_empty (path_apr, pool);
>
> if (APR_STATUS_IS_SUCCESS (status))
> *is_empty_p = TRUE;
> Index: subversion/libsvn_subr/sorts.c
> ===================================================================
> --- subversion/libsvn_subr/sorts.c (revision 8032)
> +++ subversion/libsvn_subr/sorts.c (working copy)
> @@ -31,7 +31,7 @@
>
>
>
> -/*** apr_hash_sorted_keys() ***/
> +/*** svn_sort_hash() ***/
>
> /* (Should this be a permanent part of APR?)
>
> @@ -84,14 +84,11 @@
> }
>
>
> -#ifndef apr_hash_sort_keys
> -
> -/* see svn_sorts.h for documentation */
> apr_array_header_t *
> -apr_hash_sorted_keys (apr_hash_t *ht,
> - int (*comparison_func) (const svn_item_t *,
> - const svn_item_t *),
> - apr_pool_t *pool)
> +svn_sort_hash (apr_hash_t *ht,
> + int (*comparison_func) (const svn_item_t *,
> + const svn_item_t *),
> + apr_pool_t *pool)
> {
> apr_hash_index_t *hi;
> apr_array_header_t *ary;
> @@ -113,7 +110,6 @@
>
> return ary;
> }
> -#endif /* apr_hash_sort_keys */
>
>
>
> @@ -297,21 +293,3 @@
>
> return svn_prop_is_svn_prop (propname);
> }
> -
> -
> -void *
> -apr_array_prepend (apr_array_header_t *arr)
> -{
> - /* Call apr_array_push() to ensure that enough room has been
> - alloced. */
> - apr_array_push (arr);
> -
> - /* Now, shift all the things in the array down one spot. */
> - memmove (arr->elts + arr->elt_size,
> - arr->elts,
> - ((arr->nelts - 1) * arr->elt_size));
> -
> - /* Finally, return the pointer to the first array member so our
> - caller could put stuff there. */
> - return arr->elts;
> -}
> Index: subversion/libsvn_client/blame.c
> ===================================================================
> --- subversion/libsvn_client/blame.c (revision 8032)
> +++ subversion/libsvn_client/blame.c (working copy)
> @@ -269,8 +269,8 @@
> apr_array_header_t *paths;
>
> /* Build a sorted list of the changed paths. */
> - paths = apr_hash_sorted_keys (changed_paths,
> - svn_sort_compare_items_as_paths, pool);
> + paths = svn_sort_hash (changed_paths,
> + svn_sort_compare_items_as_paths, pool);
>
> /* Now, walk the list of paths backwards, looking a parent of
> our path that has copyfrom information. */
> Index: subversion/bindings/java/javahl/native/SVNClient.cpp
> ===================================================================
> --- subversion/bindings/java/javahl/native/SVNClient.cpp (revision 8032)
> +++ subversion/bindings/java/javahl/native/SVNClient.cpp (working copy)
> @@ -146,7 +146,7 @@
> if (Err == NULL)
> {
> apr_array_header_t *array =
> - apr_hash_sorted_keys (dirents, svn_sort_compare_items_as_paths,
> + svn_sort_hash (dirents, svn_sort_compare_items_as_paths,
> subPool.pool());
>
> // create the array of DirEntry
> Index: subversion/mod_dav_svn/repos.c
> ===================================================================
> --- subversion/mod_dav_svn/repos.c (revision 8032)
> +++ subversion/mod_dav_svn/repos.c (working copy)
> @@ -2001,8 +2001,8 @@
> }
>
> /* get a sorted list of the entries */
> - sorted = apr_hash_sorted_keys(entries, svn_sort_compare_items_as_paths,
> - resource->pool);
> + sorted = svn_sort_hash(entries, svn_sort_compare_items_as_paths,
> + resource->pool);
>
> entry_pool = svn_pool_create(resource->pool);
>
> Index: subversion/clients/cmdline/ls-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/ls-cmd.c (revision 8032)
> +++ subversion/clients/cmdline/ls-cmd.c (working copy)
> @@ -49,7 +49,7 @@
> apr_array_header_t *array;
> int i;
>
> - array = apr_hash_sorted_keys (dirents, compare_items_as_paths, pool);
> + array = svn_sort_hash (dirents, compare_items_as_paths, pool);
>
> for (i = 0; i < array->nelts; ++i)
> {
> Index: subversion/clients/cmdline/log-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/log-cmd.c (revision 8032)
> +++ subversion/clients/cmdline/log-cmd.c (working copy)
> @@ -262,9 +262,8 @@
> int i;
>
> /* Get an array of sorted hash keys. */
> - sorted_paths = apr_hash_sorted_keys (changed_paths,
> - svn_sort_compare_items_as_paths,
> - pool);
> + sorted_paths = svn_sort_hash (changed_paths,
> + svn_sort_compare_items_as_paths, pool);
>
> SVN_ERR (svn_stream_printf (lb->out, pool,
> "Changed paths:" APR_EOL_STR));
> Index: subversion/libsvn_repos/load.c
> ===================================================================
> --- subversion/libsvn_repos/load.c (revision 8032)
> +++ subversion/libsvn_repos/load.c (working copy)
> @@ -508,7 +508,7 @@
> SVN_ERR (parse_fns->remove_node_props (node_baton));
>
> SVN_ERR (parse_property_block (stream,
> - apr_atoui64 (valstr),
> + svn_atoui64 (valstr),
> parse_fns,
> found_node ? node_baton : rev_baton,
> found_node,
> @@ -521,7 +521,7 @@
> APR_HASH_KEY_STRING)))
> {
> SVN_ERR (parse_text_block (stream,
> - apr_atoui64 (valstr),
> + svn_atoui64 (valstr),
> parse_fns,
> found_node ? node_baton : rev_baton,
> buffer,
> Index: subversion/libsvn_ra_dav/fetch.c
> ===================================================================
> --- subversion/libsvn_ra_dav/fetch.c (revision 8032)
> +++ subversion/libsvn_ra_dav/fetch.c (working copy)
> @@ -927,7 +927,7 @@
> if (propval == NULL)
> entry->size = 0;
> else
> - entry->size = apr_atoui64(propval->data);
> + entry->size = svn_atoui64(propval->data);
>
> /* does this resource contain any 'svn' or 'custom' properties,
> i.e. ones actually created and set by the user? */
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Dec 19 19:20:36 2003

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.