diff mbox series

[01/10] name-rev: rewrite create_or_update_name()

Message ID b8f6a47e-1cbe-b2c7-cdde-ff2dc28af2b2@web.de
State New, archived
Headers show
Series name-rev: improve memory usage | expand

Commit Message

René Scharfe Feb. 4, 2020, 9:14 p.m. UTC
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

Comments

Derrick Stolee Feb. 5, 2020, 2 a.m. UTC | #1
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
Taylor Blau Feb. 5, 2020, 2:35 a.m. UTC | #2
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
Andrei Rybak Feb. 5, 2020, 4:45 p.m. UTC | #3
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.
René Scharfe Feb. 5, 2020, 4:47 p.m. UTC | #4
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 mbox series

Patch

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,