Re: [PATCH 11/11] sequencer: stop pretending that an assignment is a condition
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- May 16, 2025, 10:13 UTC
- Message-ID
- <8641d540-516f-4dd2-bafa-cbc2568b5281@crinan.ddns.net>
- In-Reply-To
- <7a54005bd26ac17cb6d99a2e18932f97575d4aca.1747314709.git.gitgitgadget@gmail.com>
Hi Johannes
Thanks for cleaning this up - I'm not sure why I didn't just write something like this in the first place.
Best Wishes
Phillip
On 15/05/2025 14:11, Johannes Schindelin via GitGitGadget wrote:
Show 45 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 3e81bccdf3 (sequencer: factor out todo command name parsing,
> 2019-06-27), a `return` statement was introduced that basically was a
> long sequence of conditions, combined with `&&`, except for the last
> condition which is not really a condition but an assignment.
>
> The point of this construct was to return 1 (i.e. `true`) from the
> function if all of those conditions held true, and also assign the `bol`
> pointer to the end of the parsed command.
>
> Some static analyzers are really unhappy about such constructs. And
> human readers are at least puzzled, if not confused, by seeing a single
> `=` inside a chain of conditions where they would have expected to see
> `==` instead and, based on experience, immediately suspect a typo.
>
> Let's help all of this by turning this into the more verbose, more
> readable form of an `if` construct that both assigns the pointer as well
> as returns 1 if all of the conditions hold true.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index b5c4043757e9..e5e3bc6fa5ea 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2600,9 +2600,12 @@ static int is_command(enum todo_command command, const char **bol)
> const char nick = todo_command_info[command].c;
> const char *p = *bol;
>
> - return (skip_prefix(p, str, &p) || (nick && *p++ == nick)) &&
> - (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p) &&
> - (*bol = p);
> + if ((skip_prefix(p, str, &p) || (nick && *p++ == nick)) &&
> + (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p)) {
> + *bol = p;
> + return 1;
> + }
> + return 0;
> }
>
> static int check_label_or_ref_arg(enum todo_command command, const char *arg)