Julian Foad wrote:
> 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.
What we have at the moment is a patchwork quilt of different styles :-(
But, brane and lundblad helped me to understand on #svn-dev, so I will be
fixing this up per lundblad's recommendation above.
>> [[[
>> 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.
Noted.
>> (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".
Oops!
>> (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.
Yes, but the fact that a deprecated function is _not_ being turned into a
compat wrapper is noteworthy.
>> (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".
Noted.
>> - 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.
Yikes! Thanks for catching that!
>> +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.
Done.
>> + (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.
Done.
>> +
>> + 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".)
Grr indeed!
I just assumed "compare" meant a negative/zero/positive return, in the style
of "strcmp".
Fixed.
OK, now since some pretty non-trivial errors have been found, I'm going to
give the whole thing a careful read through before I post a new version.
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jun 11 21:26:45 2005