[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: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2003-12-20 01:34:31 CET

kfogel@collab.net wrote:
> Julian Foad <julianfoad@btopenworld.com> writes:
>
>>Stop defining Subversion identifiers in APR's name space.
>> (apr_hash_sorted_keys): Rename to "svn_sort_hash".
>> (apr_atoui64): Rename this macro to "svn_atoui64".

In this revised patch I have used "svn_sort__hash" and "svn__atoui64".

> 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.

I have noted in the header files that they are private and referred to issue #1644. I have not removed the Doxygen formatting as I'm not sure whether that's for the best; please do so if you're sure.

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

Renamed to "svn_sort__item_t" in this patch. (Have not renamed "svn_sort_compare_items_as_paths" but it is implicitly private because it uses a private type for its arguments. Sure, explicit would be better, but another time.)

Please feel free to modify and/or commit this if you don't hear from me in the next day or so.

- Julian

Stop defining Subversion identifiers in APR's name space (issue #1644).

* subversion/include/svn_sorts.h
* subversion/libsvn_subr/sorts.c
  (svn_sort__item_t): New name for "svn_item_t".
  (svn_sort__hash): New name for "apr_hash_sorted_keys".
  (apr_array_prepend): Delete, as it was unused.

* subversion/include/svn_types.h
  (svn__atoui64): New name for "apr_atoui64".

* subversion/bindings/java/javahl/native/SVNClient.cpp (list):
* subversion/clients/cmdline/log-cmd.c (log_message_receiver):
* subversion/clients/cmdline/ls-cmd.c (print_dirents):
* subversion/libsvn_client/blame.c (log_message_receiver):
* subversion/libsvn_fs/util/fs_skels.c (svn_fs__parse_representation_skel):
* subversion/libsvn_ra_dav/fetch.c (svn_ra_dav__get_dir):
* subversion/libsvn_repos/load.c (svn_repos_parse_dumpstream):
* subversion/mod_dav_svn/repos.c (dav_svn_deliver):
  Adjust references accordingly.

Index: subversion/include/svn_sorts.h
===================================================================
--- subversion/include/svn_sorts.h (revision 8049)
+++ subversion/include/svn_sorts.h (working copy)
@@ -34,7 +34,9 @@
 
 
 
-/** This structure is used to hold a key/value from a hash table */
+/** This structure is used to hold a key/value from a hash table.
+ * NOTE: Private. For use by Subversion's own code only. See issue #1644.
+ */
 typedef struct {
   /** pointer to the key */
   const void *key;
@@ -44,10 +46,10 @@
 
   /** pointer to the value */
   void *value;
-} svn_item_t;
+} svn_sort__item_t;
 
 
-/** Compare two @c svn_item_t's, returning an integer greater than,
+/** Compare two @c svn_sort__item_t's, returning an integer greater than,
  * equal to, or less than 0, according as @a a is greater than, equal to,
  * or less than @a b.
  *
@@ -56,13 +58,13 @@
  * 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.
  */
-int svn_sort_compare_items_as_paths (const svn_item_t *a, const svn_item_t *b);
+int svn_sort_compare_items_as_paths (const svn_sort__item_t *a,
+ const svn_sort__item_t *b);
 
 
 /** Compare two @c svn_revnum_t's, returning an integer greater than, equal
@@ -77,42 +79,28 @@
 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
- * and @a item->size are the hash key, and @a item->data points to
+ * containing @c svn_sort__item_t structures holding those keys and values
+ * (i.e. for each @c svn_sort__item_t @a item in the returned array,
+ * @a item->key and @a item->size are the hash key, and @a item->data points to
  * the hash value).
  *
  * Storage is shared with the original hash, not copied.
  *
- * @a comparison_func should take two @c svn_item_t's and return an integer
- * greater than, equal to, or less than 0, according as the first item
+ * @a comparison_func should take two @c svn_sort__item_t's and return an
+ * integer greater than, equal to, or less than 0, according as the first item
  * is greater than, equal to, or less than the second.
  *
- * NOTE:
- * 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 */
-
-#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: Private. For use by Subversion's own code only. See issue #1644.
  *
- * NOTE:
- * This function should go over to APR.
+ * NOTE: This function and the @a svn_sort__item_t should go over to APR.
  */
-void *apr_array_prepend (apr_array_header_t *arr);
-#endif /* apr_array_prepend */
+apr_array_header_t *
+svn_sort__hash (apr_hash_t *ht,
+ int (*comparison_func) (const svn_sort__item_t *,
+ const svn_sort__item_t *),
+ apr_pool_t *pool);
+
 
 #ifdef __cplusplus
 }
Index: subversion/include/svn_ra_svn.h
===================================================================
--- subversion/include/svn_ra_svn.h (revision 8049)
+++ subversion/include/svn_ra_svn.h (working copy)
@@ -210,7 +210,7 @@
 svn_error_t *svn_ra_svn_skip_leading_garbage(svn_ra_svn_conn_t *conn,
                                              apr_pool_t *pool);
 
-/** Parse an array of @c svn_item_t structures as a tuple, using a
+/** Parse an array of @c svn_sort__item_t structures as a tuple, using a
  * printf-like interface. The format string @a fmt may contain:
  *
  *<pre>
Index: subversion/include/svn_types.h
===================================================================
--- subversion/include/svn_types.h (revision 8049)
+++ subversion/include/svn_types.h (working copy)
@@ -122,8 +122,10 @@
 /** In @c printf()-style functions, format file sizes using this. */
 #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))
+/* Parse a base-10 numeric string into a 64-bit unsigned numeric value. */
+/* NOTE: Private. For use by Subversion's own code only. See issue #1644. */
+/* FIXME: APR should supply a function to do this, such as "apr_atoui64". */
+#define svn__atoui64(X) ((apr_uint64_t) apr_atoi64(X))
 
 
 /** YABT: Yet Another Boolean Type */
Index: subversion/libsvn_fs/util/fs_skels.c
===================================================================
--- subversion/libsvn_fs/util/fs_skels.c (revision 8049)
+++ 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/sorts.c
===================================================================
--- subversion/libsvn_subr/sorts.c (revision 8049)
+++ 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?)
 
@@ -51,7 +51,7 @@
 
    Therefore, it makes sense to store pointers to {void *, size_t}
    structures in our array. No such apr object exists... BUT... if we
- can use a new type svn_item_t which contains {char *, size_t, void
+ can use a new type svn_sort__item_t which contains {char *, size_t, void
    *}. If store these objects in our array, we get the hash value
    *for free*. When looping over the final array, we don't need to
    call apr_hash_get(). Major bonus!
@@ -59,7 +59,8 @@
 
 
 int
-svn_sort_compare_items_as_paths (const svn_item_t *a, const svn_item_t *b)
+svn_sort_compare_items_as_paths (const svn_sort__item_t *a,
+ const svn_sort__item_t *b)
 {
   const char *astr, *bstr;
 
@@ -84,25 +85,22 @@
 }
 
 
-#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_sort__item_t *,
+ const svn_sort__item_t *),
+ apr_pool_t *pool)
 {
   apr_hash_index_t *hi;
   apr_array_header_t *ary;
 
   /* allocate an array with only one element to begin with. */
- ary = apr_array_make (pool, 1, sizeof(svn_item_t));
+ ary = apr_array_make (pool, 1, sizeof(svn_sort__item_t));
 
   /* loop over hash table and push all keys into the array */
   for (hi = apr_hash_first (pool, ht); hi; hi = apr_hash_next (hi))
     {
- svn_item_t *item = apr_array_push (ary);
+ svn_sort__item_t *item = apr_array_push (ary);
 
       apr_hash_this (hi, &item->key, &item->klen, &item->value);
     }
@@ -113,7 +111,6 @@
 
   return ary;
 }
-#endif /* apr_hash_sort_keys */
 
 
 
@@ -297,21 +294,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 8049)
+++ subversion/libsvn_client/blame.c (working copy)
@@ -273,14 +273,15 @@
       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. */
       for (i = paths->nelts; i > 0; i--)
         {
- svn_item_t item = APR_ARRAY_IDX (paths, i - 1, svn_item_t);
+ svn_sort__item_t item = APR_ARRAY_IDX (paths, i - 1,
+ svn_sort__item_t);
           const char *ch_path = item.key;
           int len = strlen (ch_path);
 
Index: subversion/bindings/java/javahl/native/SVNClient.cpp
===================================================================
--- subversion/bindings/java/javahl/native/SVNClient.cpp (revision 8049)
+++ 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
@@ -169,10 +169,10 @@
 
                 for (int i = 0; i < array->nelts; i++)
                 {
- const svn_item_t *item;
+ const svn_sort__item_t *item;
                         svn_dirent_t *dirent = NULL;
 
- item = &APR_ARRAY_IDX (array, i, const svn_item_t);
+ item = &APR_ARRAY_IDX (array, i, const svn_sort__item_t);
                         dirent = (svn_dirent_t *) item->value;
 
                         jobject obj = createJavaDirEntry((const char *)item->key, dirent);
Index: subversion/mod_dav_svn/repos.c
===================================================================
--- subversion/mod_dav_svn/repos.c (revision 8049)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -2001,14 +2001,15 @@
       }
 
     /* 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);
 
     for (i = 0; i < sorted->nelts; ++i)
       {
- const svn_item_t *item = &APR_ARRAY_IDX(sorted, i, const svn_item_t);
+ const svn_sort__item_t *item = &APR_ARRAY_IDX(sorted, i,
+ const svn_sort__item_t);
         const svn_fs_dirent_t *entry = item->value;
         const char *name = item->key;
         const char *href = name;
Index: subversion/clients/cmdline/ls-cmd.c
===================================================================
--- subversion/clients/cmdline/ls-cmd.c (revision 8049)
+++ subversion/clients/cmdline/ls-cmd.c (working copy)
@@ -36,7 +36,7 @@
 /*** Code. ***/
 
 static int
-compare_items_as_paths (const svn_item_t *a, const svn_item_t *b)
+compare_items_as_paths (const svn_sort__item_t *a, const svn_sort__item_t *b)
 {
   return svn_path_compare_paths ((const char *)a->key, (const char *)b->key);
 }
@@ -49,15 +49,15 @@
   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)
     {
       const char *utf8_entryname, *stdout_entryname;
       svn_dirent_t *dirent;
- svn_item_t *item;
+ svn_sort__item_t *item;
      
- item = &APR_ARRAY_IDX (array, i, svn_item_t);
+ item = &APR_ARRAY_IDX (array, i, svn_sort__item_t);
 
       utf8_entryname = item->key;
 
Index: subversion/clients/cmdline/log-cmd.c
===================================================================
--- subversion/clients/cmdline/log-cmd.c (revision 8049)
+++ subversion/clients/cmdline/log-cmd.c (working copy)
@@ -262,15 +262,15 @@
       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));
       for (i = 0; i < sorted_paths->nelts; i++)
         {
- svn_item_t *item = &(APR_ARRAY_IDX (sorted_paths, i, svn_item_t));
+ svn_sort__item_t *item = &(APR_ARRAY_IDX (sorted_paths, i,
+ svn_sort__item_t));
           const char *path_stdout, *path = item->key;
           svn_log_changed_path_t *log_item
             = apr_hash_get (changed_paths, item->key, item->klen);
Index: subversion/libsvn_repos/load.c
===================================================================
--- subversion/libsvn_repos/load.c (revision 8049)
+++ 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 8049)
+++ subversion/libsvn_ra_dav/fetch.c (working copy)
@@ -928,7 +928,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
Received on Sat Dec 20 01:34:04 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.