[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: wget2 | Draft: Small fixes (!505)
From: |
Avinash Sonawane (@rootkea) |
Subject: |
Re: wget2 | Draft: Small fixes (!505) |
Date: |
Mon, 06 Jun 2022 15:01:53 +0000 |
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.
- Re: wget2 | Draft: Small fixes (!505), Avinash Sonawane (@rootkea), 2022/06/06
- Re: wget2 | Draft: Small fixes (!505), Avinash Sonawane (@rootkea), 2022/06/06
- Re: wget2 | Draft: Small fixes (!505), Avinash Sonawane (@rootkea), 2022/06/06
- Re: wget2 | Draft: Small fixes (!505), Avinash Sonawane (@rootkea), 2022/06/06
- Re: wget2 | Draft: Small fixes (!505),
Avinash Sonawane (@rootkea) <=
- Re: wget2 | Draft: Small fixes (!505), Avinash Sonawane (@rootkea), 2022/06/07
- Re: wget2 | Draft: Small fixes (!505), Avinash Sonawane (@rootkea), 2022/06/22
- Re: wget2 | Draft: Small fixes (!505), Avinash Sonawane (@rootkea), 2022/06/23
- Re: wget2 | Draft: Small fixes (!505), @rockdaboot, 2022/06/25
- Re: wget2 | Draft: Small fixes (!505), Avinash Sonawane (@rootkea), 2022/06/25
- Re: wget2 | Draft: Small fixes (!505), @rockdaboot, 2022/06/25
- Re: wget2 | Draft: Small fixes (!505), @rockdaboot, 2022/06/25