On Mon, Feb 26, 2018 at 5:49 PM, Miguel Ojeda
<miguel.ojeda.sando...@gmail.com> wrote:
> On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel <ra...@robertabel.eu> wrote:
>> NUL-terminate each individual number to be parsed.
>> To do this, the next command character and a pointer to its argument
>> are found and stored. The command character is then overwritten by NUL
>> before kstr* functions are called on the buffer.
>
> It would be useful to have this description in the code itself as a comment.
>
>>
>> Signed-off-by: Robert Abel <ra...@robertabel.eu>
>> ---
>> drivers/auxdisplay/charlcd.c | 53
>> ++++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 44 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
>> index a3d364e6c666..24cabe88c7f0 100644
>> --- a/drivers/auxdisplay/charlcd.c
>> +++ b/drivers/auxdisplay/charlcd.c
>> @@ -471,28 +471,63 @@ static inline int handle_lcd_special_code(struct
>> charlcd *lcd)
>> break;
>> }
>> case 'x': /* gotoxy : LxXXX[yYYY]; */
>> - case 'y': /* gotoxy : LyYYY[xXXX]; */
>> + case 'y': { /* gotoxy : LyYYY[xXXX]; */
>> +
>
> Extra empty line.
>
>> + char* nxt_esc;
>> + char nxt_cmd;
>
> I think skipping the "e" in the names do not buy us much :)
>
>> + char cmd;
>> + struct charlcd_priv_addr tmp_addr;
>> +
>> if (!strchr(esc, ';'))
>> break;
>>
>> - while (*esc) {
>> - if (*esc == 'x') {
>> - esc++;
>> - if (kstrtoul(esc, 10, &priv->addr.x) < 0)
>> + /* sequence is processed whether legal or illegal */
>> + processed = 1;
>> +
>> + /* copy current address to temporary buffer */
>> + tmp_addr = priv->addr;
>> +
>> + nxt_cmd = *esc++;
>> + nxt_esc = esc;
>> +
>> + while ('\0' != *esc) {
>
> Please do not change the style of the code w.r.t to the rest of the
> file, which writes tests with the non-lvalue on the right-hand side
> and do not compare against '\0'. Same for the rest.
>
>> +
>
> Extra empty line.
>
>> + cmd = nxt_cmd;
>> + esc = nxt_esc;
>> + nxt_esc = strpbrk(esc, "xy;");
>> + if (NULL != nxt_esc) {
>> + nxt_cmd = *nxt_esc;
>> + /* terminate current sequence with NUL */
>> + *nxt_esc++ = '\0';
>> + }
>> +
>> + if ('x' == cmd) {
>> + if (kstrtoul(esc, 10, &tmp_addr.x) < 0)
>> break;
>> - } else if (*esc == 'y') {
>> - esc++;
>> - if (kstrtoul(esc, 10, &priv->addr.y) < 0)
>> + } else if ('y' == cmd) {
>> + if (kstrtoul(esc, 10, &tmp_addr.y) < 0)
>> break;
>> } else {
>> + /* break on unknown command or ';' */
>> break;
>> }
>
> { } not needed (your patch doesn't touch this -- just pointing it out).
Disregard this one, just checked and coding-style.rst specifies one
must use braces if the other branches need to use them.
Cheers,
Miguel
>
>> +
>> }
>>
>> + /* unknown commands in sequence will be followed by at least
>> ';' */
>> + if ('\0' != *esc)
>> + break;
>> +
>> + /* clamp new x/y coordinates */
>> + if (tmp_addr.x >= lcd->width)
>> + tmp_addr.x = lcd->width - 1;
>
> tmp_addr.x = min(tmp_addr.x, lcd->width - 1);
>
>> + tmp_addr.y %= lcd->height;
>> +
>> + priv->addr = tmp_addr;
>> charlcd_gotoxy(lcd);
>> - processed = 1;
>> break;
>> }
>> + }
>
> On a general note, the code seems a bit convoluted for what it does,
> specially without the comment written in the commit message :-) Isn't
> it simpler to use a tiny array in the stack and put the numbers to be
> converted instead of modifying the input sequence and dancing with
> pointers?
>
> Thanks for the patch!
>
> Cheers,
> Miguel
>
>>
>> /* TODO: This indent party here got ugly, clean it! */
>> /* Check whether one flag was changed */
>> --
>> 2.11.0
>>
