Re: [PATCH 16/16] add: avoid yoda conditions

From: Martin von Zweigbergk <martinvonz@gmail.com>
Date: Oct 31, 2013, 19:48 UTC
Message-ID: <CANiSa6i5z8Z9HPzsUWTh8U2HXc9p6MPgQjJ7K6KSDw8FXtFyww@mail.gmail.com>
In-Reply-To: <1383211547-9145-17-git-send-email-felipe.contreras@gmail.com>

I was recently confused by the yoda condition in this block of code from [1]

+ for (i = 0; i < revs.nr; i++)
+ if (&bases->item->object == &revs.commit[i]->object)
+ break; /* found */
+ if (revs.nr <= i)

I think I was particularly surprised because it came so soon after the "i < revs.nr". I didn't bother commenting because it seemed too subjective and the code base has tons of these. Something as simple as

  git grep '[0-9] [<>]' *.c

finds a bunch (probably with lots of false positives and negatives).

I guess what I'm trying to say is that either we accept them and get used to reading them without being surprised, or we can change a bit more than one at a time perhaps? I understand that this was an occurrence you just happened to run into, and I'm not saying that a patch has to deal with _all_ occurrences. I'm more just wondering if we want mention our position, whatever it is, in CodingGuidelines.

Martin

[1] http://thread.gmane.org/gmane.comp.version-control.git/236252/focus=236716

On Thu, Oct 31, 2013 at 2:25 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 25 quoted lines

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/add.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 226f758..9b30356 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -429,7 +429,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>         argc--;
>         argv++;
>
> -       if (0 <= addremove_explicit)
> +       if (addremove_explicit >= 0)
>                 addremove = addremove_explicit;
>         else if (take_worktree_changes && ADDREMOVE_DEFAULT)
>                 addremove = 0; /* "-u" was given but not "-A" */
> --
> 1.8.4.2+fc1
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html