Re: [PATCH 16/16] add: avoid yoda conditions
On Thu, Oct 31, 2013 at 2:31 PM, Junio C Hamano wrote: > I agree that there is no justification to write "if 0 == something", > when "if something == 0" suffices. The latter reads better and that > is why the phrase "yoda condition" was invented. > > But the situation is different when both sides are not constants, > and especially when "<" and "<=" are involved.. To me revs.nr is virtually a constant, I'm comparing i to revs.nr, not the other way around. I believe I explained this already, but here it goes again: if (1.60 < john.size) This makes no sense, "if 1.69 is smaller than john"? The situation doesn't change when you use a variable: if (size_limit < john.size) Translates to "if size limit is smaller than john", and still makes no sense. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/16] add: avoid yoda conditions
Martin von Zweigbergk writes: > 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. That follows "visual/textual order should follow the actual ordering" principle. Think of a number-line you learn in elementary school arithmetic class, and try to place revs.nr and i on it. I agree that there is no justification to write "if 0 == something", when "if something == 0" suffices. The latter reads better and that is why the phrase "yoda condition" was invented. But the situation is different when both sides are not constants, and especially when "<" and "<=" are involved.. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/16] add: avoid yoda conditions
On Thu, Oct 31, 2013 at 1:48 PM, Martin von Zweigbergk wrote: > 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. Yes, I'm all in favor of updating CodingGuidelines with that. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/16] add: avoid yoda conditions
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 wrote: > Signed-off-by: Felipe Contreras > --- > 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 16/16] add: avoid yoda conditions
Signed-off-by: Felipe Contreras --- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
