Re: svn commit: rev 849 - trunk/subversion/include trunk/subversion/libsvn_wc trunk/subversion/clients/cmdline

On Fri, Jan 11, 2002 at 02:18:11PM -0600, sussman@tigris.org wrote:
>...
> +static svn_error_t *
> +expand_keyword (char **revision,
> + char **date,
> + char **author,
> + char **url,

See my earlier email about introducing svn_keyword_set_t ...

>...
> + apr_hash_get (entry->attributes,
> + SVN_ENTRY_ATTR_COMMITTED_REV,
> + strlen(SVN_ENTRY_ATTR_COMMITTED_REV));

argh... please use APR_HASH_KEY_STRING here. A typo on the symbol in the
strlen() could really mess things up.

>...
> + if (! value)
> + /* We found a recognized keyword, so it needs to be expanded
> + no matter what. If the expansion value isn't available,
> + we at least send back an empty string. */
> + *revision = apr_pstrdup (pool, "");

If the keyword result strings were declared "const" (you don't truly expect
a caller to attempt to *change* them, do you?), then you don't have to
duplicate a darned empty string. Just say *revision = "";

Using "const" is good. If some nimrod out there feels like changing the text
*in place*, then they can make a copy for themselves and do it. The const
gives you much more flexibility in your implementation (such as the empty
constant above). Avoiding "const" when programming means you're much more
subject to the whims of your *caller* rather than what you'd like to do.

Each section of expand_keyword() is pretty simple, but it also begs to be
put into a loop and use a descriptor array:

    { SVN_KEYWORD_AUTHOR_LONG,
      SVN_KEYWORD_AUTHOR_SHORT,
      SVN_ENTRY_ATTR_LAST_AUTHOR,
      APR_XtOffset(svn_keyword_set_t, author)
      },
    ...
    { NULL } /* sentinel */

>...
> +svn_error_t *
> +svn_wc__get_keywords (char **revision,
> + char **date,
> + char **author,
> + char **url,
> + char *path,
> + char *optional_value,

Shouldn't path and optional_value be "const" ? You aren't changing them in
this function...

>...
> + if (value[offset] != NULL) /* found non-whitespace char */
> + {
> + int word_start, word_end;

Should be apr_size_t

>...
> + /* Make a temporary copy of the word */
> + discovered_word = svn_stringbuf_ncreate (value + word_start,
> + (word_end - word_start),
> + pool);

Allocate the string once, and then use _ensure/memcpy to reuse the buffer.

Something like _set(), but counted would be nice and would avoid the need to
use _ensure and a memcpy. (e.g. _nset(buf, s, n))

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org