threads / patch / 24574

patchRemove useless temporary integer in builtin/push.c

Subject: [PATCH] Remove useless temporary integer in builtin/push.c

## tl;dr

6 messages between Jul 29, 2010 and Aug 2, 2010. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Jared Hance· Jul 29, 2010, 15:59 UTC · lore

Creating a variable nr here to use throughout the function only to change refspec_nr to nr at the end, having not used refspec_nr the entire time, is rather pointless. Instead, simply increment refspec_nr.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 builtin/push.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

Show changes to builtin/push.c +3 −4

diff --git a/builtin/push.c b/builtin/push.c
index f4358b9..79d8192 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -25,10 +25,9 @@ static int refspec_nr;
 
 static void add_refspec(const char *ref)
 {
-	int nr = refspec_nr + 1;
-	refspec = xrealloc(refspec, nr * sizeof(char *));
-	refspec[nr-1] = ref;
-	refspec_nr = nr;
+	refspec_nr++;
+	refspec = xrealloc(refspec, refspec_nr * sizeof(char *));
+	refspec[refspec_nr-1] = ref;
 }
 
 static void set_refspecs(const char **refs, int nr)

-- 
1.7.2

Thomas Rast· Jul 29, 2010, 22:21 UTC · re: Jared Hance · lore

Re: [PATCH] Remove useless temporary integer in builtin/push.c

Jared Hance wrote:

Show 5 quoted lines

> Creating a variable nr here to use throughout the function only to change
> refspec_nr to nr at the end, having not used refspec_nr the entire time,
> is rather pointless. Instead, simply increment refspec_nr.
> 
> Signed-off-by: Jared Hance <jaredhance@gmail.com>

[...]

Show 7 quoted lines

> -	int nr = refspec_nr + 1;
> -	refspec = xrealloc(refspec, nr * sizeof(char *));
> -	refspec[nr-1] = ref;
> -	refspec_nr = nr;
> +	refspec_nr++;
> +	refspec = xrealloc(refspec, refspec_nr * sizeof(char *));
> +	refspec[refspec_nr-1] = ref;

While you're already here, you could switch to ALLOC_GROW instead to avoid the n**2 behaviour of xrealloc...

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Jared Hance· Jul 31, 2010, 12:54 UTC · re: Thomas Rast · lore

[PATCH 0/2] Clean up add_refspec in builtin/push.c

This patch series removes code duplication and other poor coding practices from builtin/push.c.

Jared Hance (2):
  Remove useless temporary integer in builtin/push.c
  Use ALLOC_GROW in builtin/push.c

 builtin/push.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

-- 
1.7.2

Jared Hance· Jul 31, 2010, 12:54 UTC · re: Jared Hance · lore

[PATCH 1/2] Remove useless temporary integer in builtin/push.c

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 builtin/push.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

Show changes to builtin/push.c +3 −4

diff --git a/builtin/push.c b/builtin/push.c
index f4358b9..79d8192 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -25,10 +25,9 @@ static int refspec_nr;
 
 static void add_refspec(const char *ref)
 {
-	int nr = refspec_nr + 1;
-	refspec = xrealloc(refspec, nr * sizeof(char *));
-	refspec[nr-1] = ref;
-	refspec_nr = nr;
+	refspec_nr++;
+	refspec = xrealloc(refspec, refspec_nr * sizeof(char *));
+	refspec[refspec_nr-1] = ref;
 }
 
 static void set_refspecs(const char **refs, int nr)

-- 
1.7.2

Junio C Hamano· Aug 2, 2010, 18:51 UTC · re: Jared Hance · lore

Re: [PATCH 1/2] Remove useless temporary integer in builtin/push.c

Jared Hance <jaredhance@gmail.com> writes:

> Creating a variable nr here to use throughout the function only to change
> refspec_nr to nr at the end, having not used refspec_nr the entire time,
> is rather pointless. Instead, simply increment refspec_nr.

That is something a compiler can notice and optimize out, so it byitself is not a good criteria to judge this change. The real issue is if the use of temporary makes the code easier to read or harder.

With the two patches squashed together to use ALLOC_GROW(), the result conforms to the pattern many codepaths use, and that makes it easier to read.

Will queue, with these two squashed into one commit.

Thanks.

Show 30 quoted lines

> Signed-off-by: Jared Hance <jaredhance@gmail.com>
> ---
>  builtin/push.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index f4358b9..79d8192 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -25,10 +25,9 @@ static int refspec_nr;
>  
>  static void add_refspec(const char *ref)
>  {
> -	int nr = refspec_nr + 1;
> -	refspec = xrealloc(refspec, nr * sizeof(char *));
> -	refspec[nr-1] = ref;
> -	refspec_nr = nr;
> +	refspec_nr++;
> +	refspec = xrealloc(refspec, refspec_nr * sizeof(char *));
> +	refspec[refspec_nr-1] = ref;
>  }
>  
>  static void set_refspecs(const char **refs, int nr)
> -- 
> 1.7.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jared Hance· Jul 31, 2010, 12:57 UTC · re: Jared Hance · lore

[PATCH 2/2] Use ALLOC_GROW in builtin/push.c

The current implementation of add_refspec(const char *ref) duplicates functionality found in the xalloc api. Use ALLOC_GROW instead to prevent code duplication.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 builtin/push.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Show changes to builtin/push.c +2 −1

diff --git a/builtin/push.c b/builtin/push.c
index 79d8192..0da0ec8 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -22,11 +22,12 @@ static int progress;
 
 static const char **refspec;
 static int refspec_nr;
+static size_t refspec_alloc;
 
 static void add_refspec(const char *ref)
 {
 	refspec_nr++;
-	refspec = xrealloc(refspec, refspec_nr * sizeof(char *));
+	ALLOC_GROW(refspec, refspec_nr, refspec_alloc);
 	refspec[refspec_nr-1] = ref;
 }

-- 
1.7.2