> >
> > ptr = buf;
> > while((charsize = utf8decode(ptr, &unicodep, buflen))) {
> > utf8encode(unicodep, s, UTF_SIZ);
> > tputc(s, charsize);
> > ptr += charsize;
> > buflen -= charsize;
> > }
>
> Why not using the already encoded version at ptr? The utf8encode call
Totally agree. I think utf8encode is here to ensure that the string
passed to tputc is UTF_SIZ long, while the length string in ptr is
variable. I don't know if it is needed this size or not.
> >
> > if(len == 1) {
> > width = 1;
> > unicodep = ascii = *c;
> > } else {
> > utf8decode(c, &unicodep, UTF_SIZ);
> > width = wcwidth(unicodep);
> > control = ISCONTROLC1(unicodep);
> > ascii = unicodep;
> > }
>
> I can't say much about this, another way to do it is to make ISCONTROLC1
> act on the utf8 representation directly.
Yes, but you have the same problem in tcontrol(), and if you use
char * instead of integer values then you cannot use a switch, and
I think the switch is the clearest option there. But this code is
decoding again because we need the width of the character for
multicolum characters, the value of wcwidth() is also used at the
end of tputc().
> This is not a very optimal implementation. We can maybe use
> utf8decodebyte as done in the beggining utf8decode to be faster.
> The only point of utf8decode here is to validate the encoding,
> is that necessary?
No, it is not necessary, because this function is only called with
data from the line buffer (that it's already verified). If you want
send a patch for this point, it will be applied.
> If you think this will simplify the code and prevent future bugs, this
> is a good idea. In terms of performance you need to profile to be
> sure that there is indeed a problem here. Stupid code is stupid but
Yes, I think the code will be simplified, this is the main reason.
About performance, I think we are going to win something, but I am
pretty sure that this is not a bottleneck, so the difference is
not going to be significative.
> sometimes it works nicely! Also, it's not like st is doing poorly on
> the performance side (c.f. recent text editors embedded in full blown
> web browsers), and we don't expect a lot from a terminal emulator anyway.
Last time we did a performace test, st was one of the best, but
I don't know now. These performace test are not really importants,
because the common case is always waiting to the user.
I think we can create a branch and begin with these changes in
this new branch, because it is clear that this change needs a lot
of patches and maybe st is going to be instable in some moments.
Regards,
--
Roberto E. Vargas Caballero
Received on Sun Aug 24 2014 - 11:26:19 CEST