Max Bowsher wrote:
> Here we go again with the keywords-as-hash thing...
Excellent. Congratulations on making the patch so neatly scoped and
self-contained: it is nice to see it only touching subst.c and the
corresponding header svn_subst.h. Thus, I'm able to review it.
> OK, fixed all the issues except one...
>
>>> + {
>>> + const char *key;
>>> + svn_string_t *a_val, *b_val;
>>> +
>>> + apr_hash_this (hi, (void *) &key, NULL, (void *) &a_val);
>>
>> This is incorrect. You should cast to (void**), and that's not
>> conformant. Casting pointer-to-pointer-to-sometype to void** doesn't
>> work. You need two temporary void* variables as in many other places.
>
> (void**) works for me - is it a GCC extension?
> If we really can't use (void**), then I would like to use (void*) - it
> is, after all accurate, as a void* is a pointer to anything - even
> another void*.
Without looking deeper, I don't know what is best. I suggest you copy what we
do everywhere else if possible. If we later find a better way, we can change
everywhere all together.
> [[[
> Revise keywords API - represent keywords as a hash for better extensibility.
> Implement internal printf-like format characters for keyword expansion.
> A part of issue #890. Note that this only introduces the new APIs - making
> the rest of the code make use of them will be a separate commit.
> Based on a patch by John Peacock <jpeacock@rowman.com>, which was in turn
> based on a patch by "plasma" <plasmaball@pchome.com.tw>.
> Significant review from Peter Lundblad.
>
> * subversion/includes/svn_subst.h:
> (svn_subst_keywords_t): Deprecated.
> (svn_subst_build_keywords2): Interface change; hash instead of struct.
All of the new functions should be described with the word "New". Otherwise it
sounds like the interface to this function was changed, whereas really it is
the recommended interface to this functionality that has changed.
> (svn_subst_build_keywords): Deprecated.
> (svn_subst_keywords_differ2): Interface change.
> A new argument apr_pool_t *pool and use hash instead of struct.
> (svn_subst_keywords_differ): Deprecated.
> (svn_subst_translate_stream3): Interface change; hash instead of struct.
> (svn_subst_translate_stream2): Deprecated.
> (svn_subst_copy_and_translate3): Interface change; hash instead of struct.
> (svn_subst_copy_and_translate2): Deprecated.
> (svn_subst_translate_cstring2): Interface change; hash instead of struct.
> (svn_subst_translate_cstring): Deprecated.
>
> * subversion/libsvn_subr/svn_subst.c:
The file is "subst.c" not "svn_subst.c".
> (keyword_printf): New private function;
> printf-style formatting of keywords based on format strings.
> (date_prop_to_human): Swallowed by keyword_printf.
> (kwstruct_to_kwhash): New private function;
> convert keywords struct into keywords hash.
> (svn_subst_build_keywords): Convert to API compatibility wrapper.
> (svn_subst_build_keywords2): Build keywords using keyword_printf().
> (translate_keyword): Interface changes. Also, look up the keyword in the
> passed in buffer, instead of trying to translate all possibilities.
> (svn_subst_keywords_differ): Retain unchanged for API compatibility.
Don't mention that function, as it is untouched.
> (svn_subst_keywords_differ2): New function;
> compare two hashes instead of comparing individual structure elements.
> (svn_subst_translate_stream2): Convert to API compatibility wrapper.
> (svn_subst_translate_stream3): Change interface only.
> (svn_subst_translate_cstring): Convert to API compatibility wrapper.
> (svn_subst_translate_cstring2): Update function call to new API version.
> (svn_subst_copy_and_translate2): Convert to API combatibility wrapper.
> (svn_subst_copy_and_translate3): Update function call to new API version.
Reminder: say "New".
> (svn_subst_translate_string): Update function call to new API version.
> (svn_subst_detranslate_string): Update function call to new API version.
> ]]]
> Index: subversion/libsvn_subr/subst.c
> ===================================================================
> --- subversion/libsvn_subr/subst.c (revision 14802)
> +++ subversion/libsvn_subr/subst.c (working copy)
[...]
> + * %a author of this revision
> + * %b basename of the URL of this file
> + * %d short format of date of this revision
> + * %D long format of date of this revision
> + * %r number of this revision
> + * %u URL of this file
(If this were a public API, I would also expect it to accept "%%" for inserting
a literal "%" character, and document what hapens to invalid codes, but it
isn't so that's OK.)
> + *
> + * All memory is allocated out of @a pool.
> + */
> +static svn_string_t *
> +keyword_printf (const char *fmt,
> + const char *rev,
> + const char *url,
> + apr_time_t date,
> + const char *author,
> + apr_pool_t *pool)
> {
> + svn_stringbuf_t *value = svn_stringbuf_ncreate ("", 0, pool);
> + const char *cur;
> + int n;
A minor style nit: since "cur" and "n" are used locally in the body of the loop
(and not intended to preserve a value from one iteration to the next), it would
be clearer if they were declared locally in the body of the loop. But that
only becomes an actual problem when functions grow much bigger than this, so
it's not really important here.
> +
> + for (;;)
> {
> + cur = fmt;
>
[...]
> - kw->id = svn_string_createf (pool, "%s %s %s %s",
> - base_name,
> - rev,
> - human_date ? human_date : "",
> - author ? author : "");
> + id_val = keyword_printf ("%b %d %a %r", rev, url, date, author,
> + pool);
Oops... Here you seem to have changed the order of the keywords, putting the
revision last where it was previously second.
> +svn_boolean_t
> +svn_subst_keywords_differ2 (apr_hash_t *a,
> + apr_hash_t *b,
> + svn_boolean_t compare_values,
> + apr_pool_t *pool)
> +{
> + apr_hash_index_t *hi;
>
> + if ((a == NULL) && (b == NULL))
> + return FALSE;
> +
> + if (((a == NULL) && (b != NULL)) ||
> + ((a != NULL) && (b == NULL)) ||
That simplifies to:
if (((a == NULL) || (b == NULL)) ||
since we know at this point that if one of them is null the other isn't.
> + (apr_hash_count (a) != apr_hash_count (b)))
> + return TRUE;
> +
> + /* The hashes are both non-NULL, and have the same number of items.
> + * We must check that every item of A is present in B. */
> + for (hi = apr_hash_first(pool, a); hi; hi = apr_hash_next(hi))
> + {
> + const char *key;
> + svn_string_t *a_val, *b_val;
> +
> + apr_hash_this (hi, (const void **)&key, NULL, (void **)&a_val);
> + b_val = apr_hash_get (b, key, APR_HASH_KEY_STRING);
Make that more efficient by getting the key length as well, and using it
instead of APR_HASH_KEY_STRING.
> +
> + if (!b_val || (compare_values && svn_string_compare (a_val, b_val)))
I think that's wrong because svn_string_compare returns true if the strings are
equal, not if they differ. (Grrr... a Boolean function should have a name that
indicates its sense, like "string_equal", or "keywords_differ".)
> + return TRUE;
> + }
> +
> + return FALSE;
> +}
> +
Have fun!
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon May 23 23:10:38 2005