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

[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

[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

[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

[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?

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

[Mattias Andrée]

Signed-off-by: Mattias Andrée 
---
 libutil/unescape.c | 20 +---
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/libutil/unescape.c b/libutil/unescape.c
index d1503e6..aae15cf 100644
--- a/libutil/unescape.c
+++ b/libutil/unescape.c
@@ -1,3 +1,4 @@
 /* See LICENSE file for copyright and license details. */
+#include 
 #include 
 
@@ -18,8 +19,9 @@ unescape(char *s)
switch (s[i + 1]) {
case '\\': s[i] = '\\'; off++; break;
-   case '\'': s[i] = '\'', off++; break;
-   case '"':  s[i] =  '"', off++; break;
+   case '\'': s[i] = '\''; off++; break;
+   case '"':  s[i] =  '"'; off++; break;
case 'a':  s[i] = '\a'; off++; break;
case 'b':  s[i] = '\b'; off++; break;
+   case 'E':
case 'e':  s[i] =  033; off++; break;
case 'f':  s[i] = '\f'; off++; break;
@@ -31,7 +33,5 @@ unescape(char *s)
/* "\xH[H]" hexadecimal escape */
for (m = i + 2; m < i + 1 + 3 && m < len; m++)
-   if ((s[m] < '0' && s[m] > '9') &&
-   (s[m] < 'A' && s[m] > 'F') &&
-   (s[m] < 'a' && s[m] > 'f'))
+   if (!isxdigit(s[m]))
break;
if (m == i + 2)
@@ -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