Re: [hackers] [PATCH][sbase] libutil/unescape.c: add \E and simplify \x

2017-02-06 Thread Evan Gates 
On Mon, Feb 6, 2017 at 1:38 PM, Evan Gates  wrote:
> On Mon, Feb 6, 2017 at 1:35 PM, Mattias Andrée  wrote:
>>
>> I don't really have a prefers.
>
> OK, applied with that small change. Thanks.

Sorry, I just realized I was reading through these in the wrong order
and you had a v2 and v3 patch. I'm reading through v3 now and will
most likely go with that. Thanks for the simplifications.

Re: [hackers] [PATCH][sbase] libutil/unescape.c: add \E and simplify \x

2017-02-06 Thread Evan Gates 
On Mon, Feb 6, 2017 at 1:35 PM, Mattias Andrée  wrote:
>
> I don't really have a prefers.

OK, applied with that small change. Thanks.

Re: [hackers] [PATCH][sbase] libutil/unescape.c: add \E and simplify \x

2017-02-06 Thread Mattias Andrée 
On Mon, 6 Feb 2017 13:29:54 -0800
Evan Gates  wrote:

> On Sat, Feb 4, 2017 at 1:32 PM, Mattias Andrée
>  wrote:
> > @@ -39,10 +39,8 @@ unescape(char *s)
> > off += m - i - 1;
> > for (--m, q = 0, factor = 1; m
> > > i + 1; m--) {
> > -   if (s[m] >= '0' && s[m]
> > <= '9')
> > -   q += (s[m] -
> > '0') * factor;
> > -   else if (s[m] >= 'A' &&
> > s[m] <= 'F')
> > -   q += ((s[m] -
> > 'A') + 10) * factor;
> > -   else if (s[m] >= 'a' &&
> > s[m] <= 'f')
> > -   q += ((s[m] -
> > 'a') + 10) * factor;
> > +   if (isdigit(s[m]))
> > +   q += (s[m] &
> > 15) * factor;
> > +   else
> > +   q += ((s[m] &
> > 15) + 9) * factor; factor *= 16;
> > }
> > --
> > 2.11.0
> >
> >  
> 
> I think this would be clearer as:
> 
> if (isdigit(s[m]))
> q += (s[m] - '0') * factor;
> else
> q += (tolower(s[m]) - 'a' + 10) * factor;
> 
> But it is just a personal style preference. Or, if we do
> keep the bit twiddling, I highly recommend using hex
> instead of decimal, e.g. (s[m] & 0xf).
> 
> Thoughts?

I don't really have a prefers.


pgpxmMeCTy3tP.pgp
Description: OpenPGP digital signature

Re: [hackers] [PATCH][sbase] libutil/unescape.c: add \E and simplify \x

2017-02-06 Thread Evan Gates 
On Sat, Feb 4, 2017 at 1:32 PM, Mattias Andrée  wrote:
> @@ -39,10 +39,8 @@ unescape(char *s)
> off += m - i - 1;
> for (--m, q = 0, factor = 1; m > i + 1; m--) {
> -   if (s[m] >= '0' && s[m] <= '9')
> -   q += (s[m] - '0') * factor;
> -   else if (s[m] >= 'A' && s[m] <= 'F')
> -   q += ((s[m] - 'A') + 10) * factor;
> -   else if (s[m] >= 'a' && s[m] <= 'f')
> -   q += ((s[m] - 'a') + 10) * factor;
> +   if (isdigit(s[m]))
> +   q += (s[m] & 15) * factor;
> +   else
> +   q += ((s[m] & 15) + 9) * factor;
> factor *= 16;
> }
> --
> 2.11.0
>
>

I think this would be clearer as:

if (isdigit(s[m]))
q += (s[m] - '0') * factor;
else
q += (tolower(s[m]) - 'a' + 10) * factor;

But it is just a personal style preference. Or, if we do keep the bit
twiddling, I highly recommend using hex instead of decimal, e.g. (s[m]
& 0xf).

Thoughts?