Max, these are my comments so far on the three files you mentioned.
John Peacock wrote:
> [[[
> Implement printf-like format characters for keyword expansion. Change all
> other libraries to use new keyword hash functions from libsvn_subst.
> Includes a new keyword, UUID, and tests for same.
> See issue #890 for details.
>
> * subversion/includes/svn_subst.h:
> (svn_subst_keywords_t): Deprecated.
> (svn_subst_build_keywords2): Interface change. A new argument
> const char *uuid.
> (svn_subst_build_keywords): Convert to API compatibility wrapper.
> (svn_subst_keywords_differ2): Interface change. A new argument
> apr_pool_t *pool and use hash instead of struct.
> (svn_subst_keywords_differ): API compatibility wrapper.
> (svn_subst_copy_and_translate3): Interface change; hash instead of struct
> (svn_subst_copy_and_translate2): API compatibility wrapper
> (svn_subst_copy_and_translate): API compatibility wrapper
Don't mention that last one here - it is unchanged.
> (svn_subst_translate_cstring2): Interface change; hash instead of struct
> (svn_subst_translate_cstring): API compatibility wrapper
> (svn_subst_translate_stream2): Interface change; hash instead of struct
> (svn_subst_translate_stream): API compatibility wrapper
I'd write all of these deprecations as:
(svn_subst_translate_stream): Deprecated.
[...]
> ------------------------------------------------------------------------
>
> --- subversion/include/svn_subst.h (/mirror/trunk) (revision 13005)
> +++ subversion/include/svn_subst.h (/local/keywords) (revision 13005)
[...]
> @@ -86,16 +89,36 @@
> } svn_subst_keywords_t;
>
>
> -/** Fill in an <tt>svn_subst_keywords_t *</tt> @a kw with the appropriate
> - * contents given an @a keywords_string (the contents of the svn:keywords
> +/**
> + * @since New in 1.2.
> + *
> + * Fill in an <tt>apr_hash_t*</tt> @a kw with the appropriate contents
Instead:
* Set @a *kw to a new keywords hash filled with the appropriate contents
(This new version of the function allocates the hash, where the old version
filled in an existing structure.)
> + * given an @a keywords_string (the contents of the svn:keywords
> * property for the file in question), the revision @a rev, the @a url,
> - * the @a date the file was committed on, and the @a author of the last
> - * commit. Any of these can be @c NULL to indicate that the information is
> - * not present, or @c 0 for @a date.
> + * the @a date the file was committed on, the @a author of the last
> + * commit, and the @a uuid of the repository. Any of these can be @c NULL
> + * to indicate that the information is not present, or @c 0 for @a date.
> + *
> + * Both hash keys and values are (const char *) types.
> *
> * All memory is allocated out of @a pool.
> */
> svn_error_t *
> +svn_subst_build_keywords2 (apr_hash_t **kw,
> + const char *keywords_string,
> + const char *rev,
> + const char *url,
> + apr_time_t date,
> + const char *author,
> + const char *uuid,
> + apr_pool_t *pool);
> +
> +/** Similar to svn_subst_build_keywords2() except that it populates
> + * a svn_subst_keywords_t struct instead of a keywords hash.
Instead:
* an existing structure @a *kw instead of creating a keywords hash.
> + *
> + * @deprecated Provided for backward compatibility with the 1.1 API.
> + */
> +svn_error_t *
> svn_subst_build_keywords (svn_subst_keywords_t *kw,
> const char *keywords_string,
> const char *rev,
> @@ -107,6 +130,9 @@
>
> /** Return @c TRUE if @a a and @a b do not hold the same keywords.
> *
> + * @a a and @a b are hashes mapping @c (const char *) keyword names to
> + * @c (const * char *) keyword values.
Spurious star. Instead:
* @c (const char *) keyword values.
> + *
> * If @a compare_values is @c TRUE, "same" means that the @a a and @a b
> * contain exactly the same set of keywords, and the values of corresponding
> * keywords match as well. Else if @a compare_values is @c FALSE, then
> @@ -117,6 +143,19 @@
> * equivalent to holding no keywords.
> */
> svn_boolean_t
> +svn_subst_keywords_differ2 (const apr_hash_t *a,
> + const apr_hash_t *b,
> + svn_boolean_t compare_values,
> + apr_pool_t *pool);
> +
> +/** Similar to svn_subst_keywords_differ2() except that it compares
> + * two svn_subst_keywords_t structs instead of keyword hashes.
> + *
> + * Note: this function created and uses a temporary top-level pool.
> + *
> + * @deprecated Provided for backward compatibility with the 1.1 API.
> + */
> +svn_boolean_t
> svn_subst_keywords_differ (const svn_subst_keywords_t *a,
> const svn_subst_keywords_t *b,
> svn_boolean_t compare_values);
[...]
> @@ -217,6 +288,20 @@
> * copy.
> */
> svn_error_t *
> +svn_subst_translate_cstring2 (const char *src,
> + const char **dst,
> + const char *eol_str,
> + svn_boolean_t repair,
> + apr_hash_t *keywords,
> + svn_boolean_t expand,
> + apr_pool_t *pool);
> +
> +/** @deprecated Provided for backward compatibility with the 1.1 API.
> + *
> + * Similar to svn_subst_translate_cstring2() except that keywords struct
> + * is converted to keywords hash
" * Similar to svn_subst_translate_cstring2() except that it takes a
* keywords struct instead of a keywords hash."
(The fact that it converts it to a hash is an implementation detail; it could
work equally well without doing so.)
> + */
> +svn_error_t *
> svn_subst_translate_cstring (const char *src,
> const char **dst,
> const char *eol_str,
[...]
> --- subversion/libsvn_subr/subst.c (/mirror/trunk) (revision 13005)
> +++ subversion/libsvn_subr/subst.c (/local/keywords) (revision 13005)
[...]
> @@ -525,66 +720,67 @@
> const svn_subst_keywords_t *b,
> svn_boolean_t compare_values)
> {
> - if (((a == NULL) && (b == NULL)) /* no A or B */
> - /* no A, and B has no contents */
> - || ((a == NULL)
> - && (b->revision == NULL)
> - && (b->date == NULL)
> - && (b->author == NULL)
> - && (b->url == NULL))
> - /* no B, and A has no contents */
> - || ((b == NULL) && (a->revision == NULL)
> - && (a->date == NULL)
> - && (a->author == NULL)
> - && (a->url == NULL))
> - /* neither A nor B has any contents */
> - || ((a != NULL) && (b != NULL)
> - && (b->revision == NULL)
> - && (b->date == NULL)
> - && (b->author == NULL)
> - && (b->url == NULL)
> - && (a->revision == NULL)
> - && (a->date == NULL)
> - && (a->author == NULL)
> - && (a->url == NULL)))
> + svn_boolean_t result = FALSE;
> + /* we have to create a new top level pool because we have none
> + * but it is destroyed as soon as the real function is called */
> + apr_pool_t *pool = svn_pool_create (NULL);
Better:
* but it is destroyed as soon as the real function returns */
That comment is useful because it is a very unusual situation, but ...
> +
> + /* first we have to create a hash of each of the keyword struct's */
> + apr_hash_t *ahash = keywords_to_keyhash(a, pool);
> + apr_hash_t *bhash = keywords_to_keyhash(b, pool);
> +
> + /* and then call the real function */
> + result = svn_subst_keywords_differ2 (ahash, bhash, compare_values, pool);
> +
> + /* Always clean up after ourselves */
These last three one-line comments are redundant.
> + svn_pool_destroy (pool);
> + return result;
> +}
> +
> +svn_boolean_t
> +svn_subst_keywords_differ2 (const apr_hash_t *a,
> + const apr_hash_t *b,
> + svn_boolean_t compare_values,
> + apr_pool_t *pool)
> +{
> + svn_boolean_t result = FALSE;
> + apr_hash_index_t *hi;
> + apr_hash_t *lame_a, *lame_b;
> +
> + lame_a = (apr_hash_t *) a;
> + lame_b = (apr_hash_t *) b;
> +
> +
> + if (((a == NULL) && (b == NULL)) /* no A or B */
> + || ((a == NULL) && (b != NULL)) /* no A but B */
> + || ((a != NULL) && (b == NULL)) /* no B but A */
Those three lines simplify to:
if (((a == NULL) || (b == NULL)) /* no A or no B */
but actually that's not what you want because...
> + /* Unequal number of contents */
> + || (apr_hash_count(lame_a) != apr_hash_count(lame_b)))
> {
> - return FALSE;
> + return TRUE;
... you've changed the result of ((a == NULL) && (b == NULL)) here, now saying
that they differ.
> }
> - else if ((a == NULL) || (b == NULL))
> - return TRUE;
> -
> - /* Else both A and B have some keywords. */
> -
> - if ((! a->revision) != (! b->revision))
> - return TRUE;
> - else if ((compare_values && (a->revision != NULL))
> - && (strcmp (a->revision->data, b->revision->data) != 0))
> - return TRUE;
> -
> - if ((! a->date) != (! b->date))
> - return TRUE;
> - else if ((compare_values && (a->date != NULL))
> - && (strcmp (a->date->data, b->date->data) != 0))
> - return TRUE;
> -
> - if ((! a->author) != (! b->author))
> - return TRUE;
> - else if ((compare_values && (a->author != NULL))
> - && (strcmp (a->author->data, b->author->data) != 0))
> - return TRUE;
> -
> - if ((! a->url) != (! b->url))
> - return TRUE;
> - else if ((compare_values && (a->url != NULL))
> - && (strcmp (a->url->data, b->url->data) != 0))
> - return TRUE;
> -
> - /* Else we never found a difference, so they must be the same. */
> -
> - return FALSE;
> +
> + /* If compare_values is FALSE, we can say A and B are the same now. */
> + if (!compare_values)
> + return FALSE;
> +
> + /* compare_values is TRUE. Compare value by value */
> + for (hi = apr_hash_first(pool, lame_a);
> + hi && !result;
> + hi = apr_hash_next(hi))
> + {
> + const void *key;
> + apr_hash_this (hi, &key, NULL, NULL);
> + if (!svn_string_compare (apr_hash_get (lame_a, key,
> + APR_HASH_KEY_STRING),
> + apr_hash_get (lame_b, key,
> + APR_HASH_KEY_STRING)))
> + result = TRUE;
Just "return TRUE" to give a probability of an early exit if they do differ,
and to avoid the need for a local variable.
> + }
> +
> + return result;
> }
>
> -
> svn_error_t *
> svn_subst_translate_stream (svn_stream_t *s, /* src stream */
> svn_stream_t *d, /* dst stream */
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Mar 22 00:36:30 2005