| Message ID | b8f6a47e-1cbe-b2c7-cdde-ff2dc28af2b2@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | name-rev: improve memory usage | expand |
On 2/4/2020 4:14 PM, René Scharfe wrote: > This code was moved straight out of name_rev(). As such, we inherited > the "goto" to jump from an if into an else-if. We also inherited the > fact that "nothing to do -- return NULL" is handled last. > > Rewrite the function to first handle the "nothing to do" case. Then we > can handle the conditional allocation early before going on to populate > the struct. No need for goto-ing. I read this carefully and agree it is functionally equivalent. Since you are removing a goto and rearranging if/else blocks, I thought this response needed to be explicit, at least more than usual. Good work. Thanks, -Stolee
On Tue, Feb 04, 2020 at 09:00:23PM -0500, Derrick Stolee wrote: > On 2/4/2020 4:14 PM, René Scharfe wrote: > > This code was moved straight out of name_rev(). As such, we inherited > > the "goto" to jump from an if into an else-if. We also inherited the > > fact that "nothing to do -- return NULL" is handled last. > > > > Rewrite the function to first handle the "nothing to do" case. Then we > > can handle the conditional allocation early before going on to populate > > the struct. No need for goto-ing. > > I read this carefully and agree it is functionally equivalent. As did I. I originally thought that my MUA was wrapping the 'else if' line oddly, but then I discovered that it's the label's indentation that was throwing me off. In any case, this makes good sense. Well done indeed. > Since you are removing a goto and rearranging if/else blocks, I thought > this response needed to be explicit, at least more than usual. Good work. > > Thanks, > -Stolee Thanks, Taylor
Hi René, On 2020-02-04 22:14, René Scharfe wrote: > This code was moved straight out of name_rev(). As such, we inherited > the "goto" to jump from an if into an else-if. We also inherited the > fact that "nothing to do -- return NULL" is handled last. > > Rewrite the function to first handle the "nothing to do" case. Then we > can handle the conditional allocation early before going on to populate > the struct. No need for goto-ing. > > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > Original submission: > https://lore.kernel.org/git/20190922081846.14452-1-martin.agren@gmail.com/ Judging by this sign-off, link to Martin's email, and shortlog from the cover letter ... > Martin Ågren (1): > name-rev: rewrite create_or_update_name() ... it seems that "From: Martin Ågren <martin.agren@gmail.com>" is missing.
Am 05.02.20 um 17:45 schrieb Andrei Rybak: > Hi René, > > On 2020-02-04 22:14, René Scharfe wrote: >> This code was moved straight out of name_rev(). As such, we inherited >> the "goto" to jump from an if into an else-if. We also inherited the >> fact that "nothing to do -- return NULL" is handled last. >> >> Rewrite the function to first handle the "nothing to do" case. Then we >> can handle the conditional allocation early before going on to populate >> the struct. No need for goto-ing. >> >> Signed-off-by: Martin Ågren <martin.agren@gmail.com> >> --- >> Original submission: >> https://lore.kernel.org/git/20190922081846.14452-1-martin.agren@gmail.com/ > > Judging by this sign-off, link to Martin's email, and shortlog from the cover letter ... > >> Martin Ågren (1): >> name-rev: rewrite create_or_update_name() > > ... it seems that "From: Martin Ågren <martin.agren@gmail.com>" is missing. Yes, indeed. Sorry for that. :( René
diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 6b9e8c850b..c2239ac3f7 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -88,21 +88,21 @@ static struct rev_name *create_or_update_name(struct commit *commit, { struct rev_name *name = get_commit_rev_name(commit); + if (name && !is_better_name(name, taggerdate, distance, from_tag)) + return NULL; + if (name == NULL) { name = xmalloc(sizeof(*name)); set_commit_rev_name(commit, name); - goto copy_data; - } else if (is_better_name(name, taggerdate, distance, from_tag)) { -copy_data: - name->tip_name = tip_name; - name->taggerdate = taggerdate; - name->generation = generation; - name->distance = distance; - name->from_tag = from_tag; - - return name; - } else - return NULL; + } + + name->tip_name = tip_name; + name->taggerdate = taggerdate; + name->generation = generation; + name->distance = distance; + name->from_tag = from_tag; + + return name; } static void name_rev(struct commit *start_commit,
This code was moved straight out of name_rev(). As such, we inherited the "goto" to jump from an if into an else-if. We also inherited the fact that "nothing to do -- return NULL" is handled last. Rewrite the function to first handle the "nothing to do" case. Then we can handle the conditional allocation early before going on to populate the struct. No need for goto-ing. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- Original submission: https://lore.kernel.org/git/20190922081846.14452-1-martin.agren@gmail.com/ builtin/name-rev.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) -- 2.25.0