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 felipe.contre...@gmail.com wrote: Signed-off-by: Felipe Contreras felipe.contre...@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 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
Re: [PATCH 16/16] add: avoid yoda conditions
On Thu, Oct 31, 2013 at 1:48 PM, Martin von Zweigbergk martinv...@gmail.com 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
Martin von Zweigbergk martinv...@gmail.com 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 2:31 PM, Junio C Hamano gits...@pobox.com 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
