Re: svn commit: rev 5322 - in trunk/subversion: include libsvn_subr

[Greg Stein <gs...@lyra.org>]

On Fri, Mar 14, 2003 at 12:19:29AM -0600, cmpilato@tigris.org wrote:
>...
> +++ trunk/subversion/include/svn_xml.h	Fri Mar 14 00:19:06 2003
>...
> +/** Determine if a string of character @a data of length @a len is a
> + * safe bet for use with the svn_xml_escape_* functions found in this
> + * header. 
> + * 
> + * Determine if a string of character @a data of length @a len is a
> + * safe bet for use with the svn_xml_escape_* functions found in this
> + * header.  Return @c TRUE if it is, @c FALSE otherwise.

We tweaked the doxygen configuration. You no longer need to repeat the
summary within the main body of the comment text. Now, you'll just end up
with duplicate text :-)

> +++ trunk/subversion/libsvn_subr/xml.c	Fri Mar 14 00:19:06 2003
>...
> +static const int xml_char_validity[256] = {
>...
> +
> +  /* 092 */
> +  0, 0, 0, 0, 0, 0, 0, 0,   0, 0, 0, 0, 0, 0, 0, 0,

092??  Maybe 192... :-)

>...
> +svn_boolean_t
> +svn_xml_is_xml_safe (const char *data, apr_size_t len)
> +{
> +  const char *end = data + len;
> +  const char *p = data;
> +
> +  while (end > p++)
> +    {

Personally, I find this backwards. "end" is the invariant, so listing it
first is non-intuitive. The stress is on what is happening with p. Thus:

  while (p++ < end)


But whichever way you choose it, the code is incorrect. You never end up
testing data[0]. You also overrun the buffer by one character (by testing
*end). I think the loop is properly:

  for (p = data; p < end; ++p)

You could also use "while (p < end)" and put the post-increment in the test,
but that starts to become a bit less readable (imo).

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