Re: [hackers] [libgrapheme][PATCH] Simplify cp_decode and be more strict

From: Laslo Hunhold <dev_AT_frign.de>
Date: Thu, 28 May 2020 15:53:32 +0200

On Thu, 28 May 2020 13:48:18 +0200
Mattias Andrée <maandree_AT_kth.se> wrote:

Dear Mattias,

> Looks good, and I especially like the simplification it brings for
> using partially loaded strings.

I'm glad to hear that. Thanks!

> However, I have three minor comments:
>
> I preferred `lut[off].mask` over `(lut[off].upper - lut[off].lower)`.
> It is clearer what it means, and storing the mask in `lut` doesn't
> even increase its size since it is padded anyway because `mincp` is
> (atleast on x86-64 and i386) aligned to 4 bytes. An alternative,
> is to use `~lut[off].lower` which I think is clearer than
> `(lut[off].upper - lut[off].lower)`, but again, I prefer
> `lut[off].mask`. You could also write
> *cp = s[0] - lut[off].lower;
> I think this alternative is about as clear as using `lut[off].mask`.

I was first vary of this way, because it would be problematic if s[0] <
lut[off].lower, but because we check this beforehand this is possible.
I'll note it and add it later.

> In POSIX (but not Linux) `1 << 16` can be either 0, 1, or 2¹⁶,
> since `1` is an `int` which minimum width is 16, not 32. Similarly,
> `0x10FFFF` could overflow to 0xFFFF.

So would you recommend an explicit cast to uint32_t, i.e.

   (uint32_t)1 << 16

to overcome this?

> I think `(s[i] & 0xC0) != 0x80` is clearer than `!BETWEEN(s[i], 0x80,
> 0xBF)`, but since you changed this I assume you disagree.

I don't disagree either way. The comment I added above is sufficient in
terms of readability. I'm not a big fan of micro-optimizations and
prefer higher "readability". Both solutions are readable enough, given
a proper comment, but I just went with the "BETWEEN"-approach as it is
similar to how we check it earlier.

With best regards

Laslo

PS: No need to CC me, I am subscribed to the list. :P
Received on Thu May 28 2020 - 15:53:32 CEST