Thank you for the review!
>> + unsigned slen = strlen(s);
>
> When you use slen below, you multiply it by 2. Might as well do that
> multiplication once, here.
I expect the compiler to be able to manage that level of common
subexpression elimination. So it will make no difference to the
generated code. Do you think it makes the code clearer?
Ah! On second thought, I *will* do that. It's clearer if I rename
the variable:
desclen = 2+2*strlen(s);
if (desclen > 254)
desclen = 254; /* Shouldn't happen, but clamp just in case */
and move the comparison with utfmax up. (I'll also give "utfmax"
a better name.)
>> + if (slen > 126)
>> + slen = 126; /* Limit so 2 + 2*slen <= 255 */
>
> Use the min() macro.
Um... why? I prefer the asymmetrical form when there is an exceptional
condition. I find it easier to follow the "normal flow" when it's a
subset of the lines, rather than having to pick lines apart.
I agree it would be shorter to write:
unsigned slen = min(strlen(s), 126);
and don't really object, I'm just curious.
>> - for (retval = 0; *s && utfmax > 1; utfmax -= 2, retval += 2) {
>> + if (!utfmax--)
>> + goto end;
>> + *utf++ = 2 + 2*slen; /* Descriptor total length */
>> +
>> + if (!utfmax--)
>> + goto end;
>> + *utf++ = 3; /* Descriptor type 3 (string) */
>
> Although this was copied from the original code, you might as well
> clean it up. USB_DT_STRING is more informative and doesn't need a
> comment.
Definitely an improvement, and I already did it on my own, actually!
(I just had to find the right header file and preprocessor name.
I also did it to the langids[] array.)
>> +
>> + if (utfmax > 2*slen)
>> + utfmax = 2*slen;
>> +
>> + while (utfmax--) {
>> *utf++ = *s++;
>> + if (!utfmax--)
>> + break;
>> *utf++ = 0;
>> }
>> - if (utfmax > 0) {
>> - *utf = *s;
>> - ++retval;
>> - }
>> - return retval;
>> +end:
>> + return (unsigned)(utf - utf0);
>> }
>
> Is this really a cleanup? You have added a bunch of code without
> making anything significantly better, apart from that one erroneous
> test.
I think it makes the "continue until we run out of utfmax" logic
much easier to follow, because it's all done in one place with one
pattern ("if (!utfmax--)").
In general "continue until you run out of A or you run out of B" loops
are a bit messy if you don't consume equal numbers of each per loop
iteration.
"Do it 2 bytes at a time, then handle the leftover byte" would probably
be more efficient, but I don't consider this fast-path code; I'll
optimize for code size.
By moving the descriptor header into str2utf, I've lost the simplification
in this function (it's about back to about where it was), but removed a
different sort of "don't output more than len" logic (the "switch(len)")
from rh_string(). that seems like a very worthwhile cleanup.
The other nice cleanup is the elimination of "retval". By cacheing the
initial value of the "utf" pointer and returning "(utf - utf0)", I both
saved per-loop work and made it easier for the reader, who doesn't have to
keep track of as many loop counters.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
