Re: [PATCH] urlmatch: use hex2chr() in append_normalized_escapes()

From: Kyle J. McKay <mackyle@gmail.com>
Date: Jul 8, 2017, 14:28 UTC
Message-ID: <A1589486-3E84-494C-9B8D-3FB1724B3145@gmail.com>
In-Reply-To: <eb5e7bb5-d0a9-c8df-e89c-a2bd2430e8b6@web.de>

On Jul 8, 2017, at 01:59, René Scharfe wrote:

> Simplify the code by using hex2chr() to convert and check for invalid
> characters at the same time instead of doing that sequentially with
> one table lookup for each.

I think that comment may be a bit misleading as the changes are just switching from one set of inlines to another. Essentially the same sequential check takes place in the hex2chr inlined function which is being used to replace the "one table lookup for each". An optimizing compiler will likely eliminate any difference between the before and after patch versions. Nothing immediately comes to mind as an alternate comment though, so I'm not proposing any changes to the comment.

The before version only requires knowledge of the standards-defined isxdigit and the hexval function which is Git-specific, but its semantics are fairly obvious from the surrounding code.

I suspect the casual reader of the function will have to go check and see what hex2chr does exactly. For example, does it accept "0x41" or not? (It doesn't.) What does it do with a single hex digit? (An error.) It does do pretty much the same thing as the code it's replacing (although that's not immediately obvious unless you go look at it), so this seems like a reasonable change.

 From the perspective of how many characters the original is versus how
many characters the replacement is, it's certainly a simplification.

But from the perspective of a reviewer of the urlmatch functionality attempting to determine how well the code does or does not match the respective standards it requires more work. Now one must examine the hex2chr function to be certain it doesn't include any extra unwanted behavior with regards to how well urlmatch complies with the applicable standards. And in that sense it is not a simplification at all.

But that's all really just nit picking since hex2chr is a simple inlined function that's relatively easy to find (and understand).

Therefore I don't have any objections to this change.

Acked-by: Kyle J. McKay

Show 28 quoted lines

> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> urlmatch.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/urlmatch.c b/urlmatch.c
> index 4bbde924e8..3e42bd7504 100644
> --- a/urlmatch.c
> +++ b/urlmatch.c
> @@ -42,12 +42,12 @@ static int append_normalized_escapes(struct  
> strbuf *buf,
>
> 		from_len--;
> 		if (ch == '%') {
> -			if (from_len < 2 ||
> -			    !isxdigit(from[0]) ||
> -			    !isxdigit(from[1]))
> +			if (from_len < 2)
> 				return 0;
> -			ch = hexval(*from++) << 4;
> -			ch |= hexval(*from++);
> +			ch = hex2chr(from);
> +			if (ch < 0)
> +				return 0;
> +			from += 2;
> 			from_len -= 2;
> 			was_esc = 1;
> 		}