On Mon, 06 Feb 2017 15:05:32 -0800
evan.gates_AT_gmail.com (Evan Gates) wrote:
> Mattias Andrée <maandree_AT_kth.se> wrote:
>
> > + } else if (escapes[*r & 255]) {
> > + *w++ = escapes[*r++ & 255];
>
> Why do you & 255 here? I think a cast to unsigned char
> would accomplish what you are trying to do and be more
> correct (as char can default to either signed or
> unsigned). Although I may misunderstand what is going on
> here.
Yes. I used &255 because it's does clutter as much.
>
> > + q = q * 8 + (*r & 7);
>
> I think this is clearer:
>
> q = q * 8 + (*r - '0');
Go for it!
>
> > + *w++ = q > 255 ? 255 : q;
>
> Probably use MIN(q, 255) instead of the explicit ternary.
Yeah, I forgot that we have MIN macro, so I didn't change that part.
>
> > + for (q = 0, m = 2; m &&
> > isxdigit(*r); m--, r++)
> > + q = q * 16 + (*r & 15)
> > + 9 * !!isalpha(*r);
>
> I think this is clearer, even though it adds a
> conditional:
>
> q = q * 16 + isdigit(*r) ? *r - '0' : tolower(*r) - 'a' +
> 10;
I think
if (isdigit(*r))
q = q * 16 + (*r - '0');
else
q = q * 16 + (tolower(*r) - 'a') + 10;
is clearer, or at least brackets around the ternary.
>
> Great work, much much simpler implementation. Once I
> understand the &255 and we agree on the best solution for
> these few lines I'll go ahead and push this.
Received on Tue Feb 07 2017 - 00:12:59 CET