| Message ID | 20230323162235.995559-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: clarify conditionals in todo_list_to_strbuf() | expand |
On Thu, Mar 23, 2023 at 05:22:35PM +0100, Oswald Buddenhagen wrote: > Make it obvious that the two conditional branches are mutually > exclusive. > > As a drive-by, remove a pair of unnecessary braces. > > Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> > --- > sequencer.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 3be23d7ca2..9169876441 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis > if (item->command == TODO_FIXUP) { > if (item->flags & TODO_EDIT_FIXUP_MSG) > strbuf_addstr(buf, " -c"); > - else if (item->flags & TODO_REPLACE_FIXUP_MSG) { > + else if (item->flags & TODO_REPLACE_FIXUP_MSG) > strbuf_addstr(buf, " -C"); > - } > - } > - > - if (item->command == TODO_MERGE) { > + } else if (item->command == TODO_MERGE) { I dunno. I think seeing adjacent if (item->command == TODO_ABC) and if (item->command == TODO_XYZ) makes it clear that these two are mutually exclusive, since TODO_ABC != TODO_XYZ. So I don't mind the unnecessary brace cleanup, but I don't think that this adds additional clarity around these two if-statements. Specifically: why not combine these two with if-statement that proceeds it? That might look something like: if (item->command == TODO_EXEC || item->command == TODO_LABEL || item->command == TODO_RESET || item->command == TODO_UPDATE_REF) { ... } else if (item->command == TODO_FIXUP) { ... } else if (item->command == TODO_MERGE) { ... } but at that point, you might consider something like: switch (item->command) { case TODO_EXEC: case TODO_LABEL: case TODO_RESET: case TODO_UPDATE_REF: ... break; case TODO_FIXUP: ... break; case TODO_MERGE: ... break; } which is arguably clearer, but I have a hard time justifying as worthwhile. TBH, it feels like churn to me, but others may disagree and see it differently. Thanks, Taylor
On Thu, Mar 23, 2023 at 04:32:41PM -0400, Taylor Blau wrote: >I dunno. I think seeing adjacent > > if (item->command == TODO_ABC) > >and > > if (item->command == TODO_XYZ) > >makes it clear that these two are mutually exclusive, since TODO_ABC != >TODO_XYZ. > no, because you have to prove to yourself that the queried value doesn't change in between. and so does the compiler, which may fail to tail-merge the embedded strbuf_addstr() calls as a consequence. >Specifically: why not combine these two with if-statement that proceeds >it? That might look something like: [...] > i don't see what you're referring to, so i guess you got confused about the location of the code in question?
Hi Taylor & Oswald On 23/03/2023 20:32, Taylor Blau wrote: > On Thu, Mar 23, 2023 at 05:22:35PM +0100, Oswald Buddenhagen wrote: >> Make it obvious that the two conditional branches are mutually >> exclusive. >> >> As a drive-by, remove a pair of unnecessary braces. >> >> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> >> --- >> sequencer.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/sequencer.c b/sequencer.c >> index 3be23d7ca2..9169876441 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis >> if (item->command == TODO_FIXUP) { >> if (item->flags & TODO_EDIT_FIXUP_MSG) >> strbuf_addstr(buf, " -c"); >> - else if (item->flags & TODO_REPLACE_FIXUP_MSG) { >> + else if (item->flags & TODO_REPLACE_FIXUP_MSG) >> strbuf_addstr(buf, " -C"); >> - } >> - } >> - >> - if (item->command == TODO_MERGE) { >> + } else if (item->command == TODO_MERGE) { > > I dunno. I think seeing adjacent > > if (item->command == TODO_ABC) > > and > > if (item->command == TODO_XYZ) > > makes it clear that these two are mutually exclusive, since TODO_ABC != > TODO_XYZ. I agree, it is easy to see that they are testing different conditions and item->command is not mutated in between > So I don't mind the unnecessary brace cleanup, but I don't think that > this adds additional clarity around these two if-statements. > > Specifically: why not combine these two with if-statement that proceeds > it? That might look something like: I think you're looking at parse_insn_line() here rather than todo_list_to_strbuf() but your analysis of this patch still stands. Best Wishes Phillip > > if (item->command == TODO_EXEC || item->command == TODO_LABEL || > item->command == TODO_RESET || item->command == TODO_UPDATE_REF) { > ... > } else if (item->command == TODO_FIXUP) { > ... > } else if (item->command == TODO_MERGE) { > ... > } > > but at that point, you might consider something like: > > switch (item->command) { > case TODO_EXEC: > case TODO_LABEL: > case TODO_RESET: > case TODO_UPDATE_REF: > ... > break; > case TODO_FIXUP: > ... > break; > case TODO_MERGE: > ... > break; > } > > which is arguably clearer, but I have a hard time justifying as > worthwhile. TBH, it feels like churn to me, but others may disagree and > see it differently. > > Thanks, > Taylor
diff --git a/sequencer.c b/sequencer.c index 3be23d7ca2..9169876441 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis if (item->command == TODO_FIXUP) { if (item->flags & TODO_EDIT_FIXUP_MSG) strbuf_addstr(buf, " -c"); - else if (item->flags & TODO_REPLACE_FIXUP_MSG) { + else if (item->flags & TODO_REPLACE_FIXUP_MSG) strbuf_addstr(buf, " -C"); - } - } - - if (item->command == TODO_MERGE) { + } else if (item->command == TODO_MERGE) { if (item->flags & TODO_EDIT_MERGE_MSG) strbuf_addstr(buf, " -c"); else
Make it obvious that the two conditional branches are mutually exclusive. As a drive-by, remove a pair of unnecessary braces. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- sequencer.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)