Re: [PATCH] issue #2147 - v2

VK Sameer <sameer@collab.net> writes:

> +const char *
> +svn_xml_fuzzy_escape (const char *string, apr_pool_t *pool)
> +{
> + const char *end = string + strlen (string);
> + const char *p = string, *q;
> + svn_stringbuf_t *outstr;
> + char escaped_char[5]; /* 0 x \xdigit \xdigit \0 */
>
> + for (q = p; q < end; q++)
> + if (svn_ctype_iscntrl (*q))

Why is this test not the inverse of the test below?

> + break;
> +
> + /* return original string if no control characters found*/
> + if (q == end)
> + return string;
> +
> + outstr = svn_stringbuf_create ("", pool);
> + while (1)
> + {
> + q = p;
> +
> + /* Traverse till either control character or eos.
> + Skip whitespace control characters */
> + while (q < end &&
> + (svn_ctype_isspace (*q) || !svn_ctype_iscntrl (*q)))

Why is this test not the inverse of the one above?

Perhaps you could structure the code so that the test only occurs in
one place?

> + q++;
> +
> + /* copy chunk before marker */
> + svn_stringbuf_appendbytes (outstr, p, q - p);
> +
> + if (q == end)
> + break;
> +
> + /* Append the control character in 0x%x format. */
> + sprintf (escaped_char, "0x%x", *q);

Plain char could be signed and so the code has the potential to go
wrong if somebody ever modifies the isspace/iscntrl test, casting *q
to (unsigned char) is safer.

> + svn_stringbuf_appendcstr (outstr, escaped_char);
> +
> + p = q + 1;
> + }
> +
> + return outstr->data;
> +}
> +

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org