Daniel Rall wrote:
> On Thu, 22 Sep 2005, Julian Foad wrote:
>>That said, I tentatively suggest the attached patch. What do you think of
>>it?
>
> Looks good! A few comments inlined below.
Thanks.
>>+ /* For efficiency, convert an empty set of keywords to NULL. */
>>+ if (keywords && (apr_hash_count (keywords) == 0))
>
> Parens around the second condition are extraneous.
Yes, I know. This project's policy on this matter is for programmers to use
their judgement about when redundant parentheses provide a useful amount of
clarity, and to err on the side of too many, so I'll keep them. (I don't
particularly like them, but I feel that the benefit to people who do like them
outweighs the slight irritation to people who don't.)
>>+ keywords = NULL;
>>+
>> /* The docstring requires that *some* translation be requested. */
>> assert (eol_str || keywords);
>> interesting = (eol_str && keywords) ? "$\r\n" : eol_str ? "\r\n" : "$";
>>@@ -952,7 +956,7 @@ svn_subst_translate_cstring2 (const char
>> src_stringbuf = svn_stringbuf_create (src, pool);
>>
>> /* The easy way out: no translation needed, just copy. */
>>- if (! (eol_str || keywords))
>>+ if (! (eol_str || (keywords && apr_hash_count (keywords))))
>
> For consistency with the check "== 0" -- and to improve clarity by being more
> explicit -- the return value of apr_hash_count() should be checked as "> 0".
OK. I considered that too and decided it was marginally not worth it -
certainly wouldn't be if the "== 0" version above didn't exist. But since it
does and you picked up on it, I'll happily add the explicit comparison.
Thanks for the review.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Sep 22 22:51:28 2005