Hi Laslo, On Thu, 28 May 2020 15:53:32 +0200 Laslo Hunhold <[email protected]> wrote:
> On Thu, 28 May 2020 13:48:18 +0200 > Mattias Andrée <[email protected]> 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? Yes. > > > 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 > Sorry, I pressing reply to all instead of reply to list without really looking, I need to remove the former option. Regards, Mattias Andrée
