Re: wget2 | Draft: Small fixes (!505)

[Avinash Sonawane (@rootkea)]

Avinash Sonawane commented on a discussion on libwget/buffer.c: 
https://gitlab.com/gnuwget/wget2/-/merge_requests/505#note_972656742

>               char *start = buf->data;
>               char *end = start + buf->length - 1;
>  
> -             if (isspace(*end)) {
> +             /* Accessing `start - 1` is undefined so leave if `start == 
> end` */

There are couple of issues here:
1. It seems that the `if` checks (`if (isspace(*start))` and `if 
(isspace(*end))`) are there for conditional execution of the 2 statements under 
both the `for` loops. e.g. set buffer length only if there was trimming (at the 
end or at the beginning). In other words, calculate buf->length iff the loop 
was executed at least once (there was trimming).

    But the first `if` statement is insufficient since there are 2 conditions 
required for trimming to happen (`for` expressions) and we're just checking for 
1 in `if` expression. So I updated that `if` check.

2. Once we've checked the `if` expressions we're again checking the same 
variables in `for` expression. So I decided to use do-while instead of `for` to 
avoid that one redundant check. But do-while looks ugly (needs 3 lines compared 
to 1 of `for`). So may be it's okay to have one redundant check in exchange of 
clear concise code. i.e. stick to `for` instead of `do-while`?

3. Since the same checks are repeated at two places (`if` and `for`) we can 
simplify the code further in exchange of some redundant ops by simply omitting 
the `if` checks. This will make those corresponding 2 statements under both the 
`for` loops to get executed even if there was no trimming. E.g. it will 
`memmove` a string overwriting itself yielding the same string, recalculating 
`buf->length` even when buf didn't change (no trimming) etc.

```
                char *start = buf->data;
                char *end = start + buf->length - 1;
 
-               if (isspace(*end)) {
-                       /* Skip trailing spaces */
-                       for (; isspace(*end) && end >= start; end--)
-                               ;
-                       end[1] = 0;
-                       buf->length = (size_t) (end - start + 1);
-               }
-
-               if (isspace(*start)) {
-                       /* Skip leading spaces */
-                       for (; isspace(*start) && end >= start; start++)
-                               ;
-                       buf->length = (size_t) (end - start + 1);
-                       /* Include trailing 0 */
-                       memmove(buf->data, start, buf->length + 1);
-               }
+               /* Skip trailing spaces */
+               for (; isspace(*end) && end >= start; end--)
+                       ;
+               end[1] = 0;
+               buf->length = (size_t) (end - start + 1);
+
+               /* Skip leading spaces */
+               for (; isspace(*start) && end >= start; start++)
+                       ;
+               buf->length = (size_t) (end - start + 1);
+               /* Include trailing 0 */
+               memmove(buf->data, start, buf->length + 1);
        }

```

4. And last but not the least, the check in first `for`/`do-while` should be 
`start <  end` and not `start <= end` since then `end--` leads to UB when 
`start == end`.

-- 
Reply to this email directly or view it on GitLab: 
https://gitlab.com/gnuwget/wget2/-/merge_requests/505#note_972656742
You're receiving this email because of your account on gitlab.com.