| Message ID | 20220914193102.5275-2-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c7c4f7608afe811ea5c2095d93020639707a8733 |
| Headers | show |
| Series | diff-merges: minor cleanups | expand |
Sergey Organov <sorganov@gmail.com> writes: > Get rid of unneeded "else" statements in func_by_opt(). While it is true that loss of "else" will not change what the code means, this change feels subjective and I'd say it falls into "once committed, it is not worth the patch noise to go in and change" category, not a "clean-up we should do". I haven't looked at diff-merges.c for quite a while, but I did. I notice that the code is barely commented on what each helper function is supposed to do, etc., and very hard to follow. The file needs cleaning up in that area a lot more, I would say. Thanks. > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > diff-merges.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/diff-merges.c b/diff-merges.c > index 7f64156b8bfe..780ed08fc87f 100644 > --- a/diff-merges.c > +++ b/diff-merges.c > @@ -60,15 +60,15 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg) > return suppress; > if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent")) > return set_first_parent; > - else if (!strcmp(optarg, "separate")) > + if (!strcmp(optarg, "separate")) > return set_separate; > - else if (!strcmp(optarg, "c") || !strcmp(optarg, "combined")) > + if (!strcmp(optarg, "c") || !strcmp(optarg, "combined")) > return set_combined; > - else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined")) > + if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined")) > return set_dense_combined; > - else if (!strcmp(optarg, "r") || !strcmp(optarg, "remerge")) > + if (!strcmp(optarg, "r") || !strcmp(optarg, "remerge")) > return set_remerge_diff; > - else if (!strcmp(optarg, "m") || !strcmp(optarg, "on")) > + if (!strcmp(optarg, "m") || !strcmp(optarg, "on")) > return set_to_default; > return NULL; > }
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Get rid of unneeded "else" statements in func_by_opt(). > > While it is true that loss of "else" will not change what the code > means, this change feels subjective and I'd say it falls into "once > committed, it is not worth the patch noise to go in and change" > category, not a "clean-up we should do". I agree the "else" vs "no else" is subjective, but the problem in fact is that the first "if", unlike the rest of them, already had no "else", making the code inconsistent. So the fix should either be adding one "else" to the first "if", or remove all of the "else". I chose the latter, to end up with less noisy code. > > I haven't looked at diff-merges.c for quite a while, but I did. I > notice that the code is barely commented on what each helper > function is supposed to do, etc., and very hard to follow. The file > needs cleaning up in that area a lot more, I would say. I believe each helper function does exactly what its name suggests, so no comments are needed. I hate to add comments that actually just say the same as function name, so I'd rathe consider to rename some functions instead should somebody point out to problematic ones. That said, it seems Elijah Newren had not much trouble adding his --remerge-diff capability to the diff-merges code, so the code must be not that hard to follow even in its current state. Thanks, -- Sergey Organov.
Sergey Organov <sorganov@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> Get rid of unneeded "else" statements in func_by_opt(). >> >> While it is true that loss of "else" will not change what the code >> means, this change feels subjective and I'd say it falls into "once >> committed, it is not worth the patch noise to go in and change" >> category, not a "clean-up we should do". > > I agree the "else" vs "no else" is subjective, but the problem in fact > is that the first "if", unlike the rest of them, already had no "else", > making the code inconsistent. This is a static helper function about a single "optarg" string that wanted to say "switch(optarg) { case "off": ... }" but couldn't in C, and I happen to view if strcmp else if strcmp ... sequence on the same string a poor-man's substitute for such a construct. So my take of it is to call the second "if" not being "else if" a problem, not the rest of it. If we add a new condition on a different input, making it "if (x) ...; switch(optarg) { ... }" that talks about something other than optarg, then writing it all with "if" without "else if" would make it harder to see the pattern, but I do not care too deeply either way, because this is unlikely to gain any logic more involved than "switch(optarg) { ... }". > So the fix should either be adding one > "else" to the first "if", or remove all of the "else". I chose the > latter, to end up with less noisy code. Yup, see above for the reason why I would choose else-if cascade if I had to but I do not care too deeply either way in this particular case ;-)
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Junio C Hamano <gitster@pobox.com> writes: >> >>> Sergey Organov <sorganov@gmail.com> writes: >>> >>>> Get rid of unneeded "else" statements in func_by_opt(). >>> >>> While it is true that loss of "else" will not change what the code >>> means, this change feels subjective and I'd say it falls into "once >>> committed, it is not worth the patch noise to go in and change" >>> category, not a "clean-up we should do". >> >> I agree the "else" vs "no else" is subjective, but the problem in fact >> is that the first "if", unlike the rest of them, already had no "else", >> making the code inconsistent. > > This is a static helper function about a single "optarg" string that > wanted to say "switch(optarg) { case "off": ... }" but couldn't in > C, and I happen to view if strcmp else if strcmp ... sequence on the > same string a poor-man's substitute for such a construct. So my > take of it is to call the second "if" not being "else if" a problem, > not the rest of it. If we add a new condition on a different input, > making it "if (x) ...; switch(optarg) { ... }" that talks about > something other than optarg, then writing it all with "if" without > "else if" would make it harder to see the pattern, but I do not care > too deeply either way, because this is unlikely to gain any logic > more involved than "switch(optarg) { ... }". Well, I even tend to write this switch(optarg) as: if (0) ; else if(...) { ... } else if(...) { ... } to make the first actual "case" look the same way as the rest, and then through suitable defines it finally looks like: IF_SWITCH IF_CASE(...) { ... } IF_CASE(...) { ... } making the intent explicit. Just saying. Thanks, -- Sergey Organov
diff --git a/diff-merges.c b/diff-merges.c index 7f64156b8bfe..780ed08fc87f 100644 --- a/diff-merges.c +++ b/diff-merges.c @@ -60,15 +60,15 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg) return suppress; if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent")) return set_first_parent; - else if (!strcmp(optarg, "separate")) + if (!strcmp(optarg, "separate")) return set_separate; - else if (!strcmp(optarg, "c") || !strcmp(optarg, "combined")) + if (!strcmp(optarg, "c") || !strcmp(optarg, "combined")) return set_combined; - else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined")) + if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined")) return set_dense_combined; - else if (!strcmp(optarg, "r") || !strcmp(optarg, "remerge")) + if (!strcmp(optarg, "r") || !strcmp(optarg, "remerge")) return set_remerge_diff; - else if (!strcmp(optarg, "m") || !strcmp(optarg, "on")) + if (!strcmp(optarg, "m") || !strcmp(optarg, "on")) return set_to_default; return NULL; }
Get rid of unneeded "else" statements in func_by_opt(). Signed-off-by: Sergey Organov <sorganov@gmail.com> --- diff-merges.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)