threads / patch / 10969

patchAvoid recalculating filename string pointer.

Subject: [PATCH] Avoid recalculating filename string pointer.

## tl;dr

4 messages between Nov 22, 2007 and Nov 25, 2007. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

André Goddard Rosa· Nov 22, 2007, 00:59 UTC · lore

Hi, all!

    Please cc: me as I'm not subscribed. I'm sending the patch inline
only for review, probably it is mangled.
    Please use the attached patch if you agree with it. Sorry about
sending it attached.

>From b6b05d9f8d8e053df4e971cd229e03b778c4d163 Mon Sep 17 00:00:00 2001

From: Andre Goddard Rosa <andre.goddard@gmail.com>
Date: Tue, 27 Nov 2007 10:17:54 -0200
Subject: [PATCH] Avoid recalculating filename string pointer.

Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com>
---
 fast-import.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Show changes to fast-import.c +4 −2

diff --git a/fast-import.c b/fast-import.c
index 98c2bd5..2d262eb 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2304,11 +2304,13 @@ int main(int argc, const char **argv)
 		else if (!prefixcmp(a, "--export-marks="))
 			mark_file = a + 15;
 		else if (!prefixcmp(a, "--export-pack-edges=")) {
+			char *filename = a + 20;
+
 			if (pack_edges)
 				fclose(pack_edges);
-			pack_edges = fopen(a + 20, "a");
+			pack_edges = fopen(filename, "a");
 			if (!pack_edges)
-				die("Cannot open %s: %s", a + 20, strerror(errno));
+				die("Cannot open %s: %s", filename, strerror(errno));
 		} else if (!strcmp(a, "--force"))
 			force_update = 1;
 		else if (!strcmp(a, "--quiet"))

-- 
1.5.3.6.861.gd794-dirty


From b6b05d9f8d8e053df4e971cd229e03b778c4d163 Mon Sep 17 00:00:00 2001
From: Andre Goddard Rosa <andre.goddard@gmail.com>
Date: Tue, 27 Nov 2007 10:17:54 -0200
Subject: [PATCH] Avoid recalculating filename string pointer.

Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com>
---
 fast-import.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 98c2bd5..2d262eb 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2304,11 +2304,13 @@ int main(int argc, const char **argv)
 		else if (!prefixcmp(a, "--export-marks="))
 			mark_file = a + 15;
 		else if (!prefixcmp(a, "--export-pack-edges=")) {
+			char *filename = a + 20;
+
 			if (pack_edges)
 				fclose(pack_edges);
-			pack_edges = fopen(a + 20, "a");
+			pack_edges = fopen(filename, "a");
 			if (!pack_edges)
-				die("Cannot open %s: %s", a + 20, strerror(errno));
+				die("Cannot open %s: %s", filename, strerror(errno));
 		} else if (!strcmp(a, "--force"))
 			force_update = 1;
 		else if (!strcmp(a, "--quiet"))
-- 
1.5.3.6.861.gd794-dirty

Mike Hommey· Nov 22, 2007, 19:54 UTC · re: André Goddard Rosa · lore

Re: [PATCH] Avoid recalculating filename string pointer.

On Wed, Nov 21, 2007 at 10:59:41PM -0200, André Goddard Rosa wrote:

Show 18 quoted lines

> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2304,11 +2304,13 @@ int main(int argc, const char **argv)
>  		else if (!prefixcmp(a, "--export-marks="))
>  			mark_file = a + 15;
>  		else if (!prefixcmp(a, "--export-pack-edges=")) {
> +			char *filename = a + 20;
> +
>  			if (pack_edges)
>  				fclose(pack_edges);
> -			pack_edges = fopen(a + 20, "a");
> +			pack_edges = fopen(filename, "a");
>  			if (!pack_edges)
> -				die("Cannot open %s: %s", a + 20, strerror(errno));
> +				die("Cannot open %s: %s", filename, strerror(errno));
>  		} else if (!strcmp(a, "--force"))
>  			force_update = 1;
>  		else if (!strcmp(a, "--quiet"))

Normally, the compiler takes care of such optimizations. It actually takes care of it much better than you can do yourself, and doing it yourself can even sometimes generate less optimized code because it gets in the compiler optimizations'way.

Mike

Junio C Hamano· Nov 22, 2007, 20:21 UTC · re: Mike Hommey · lore

Re: [PATCH] Avoid recalculating filename string pointer.

Mike Hommey <mh@glandium.org> writes:

Show 24 quoted lines

> On Wed, Nov 21, 2007 at 10:59:41PM -0200, André Goddard Rosa wrote:
>> --- a/fast-import.c
>> +++ b/fast-import.c
>> @@ -2304,11 +2304,13 @@ int main(int argc, const char **argv)
>>  		else if (!prefixcmp(a, "--export-marks="))
>>  			mark_file = a + 15;
>>  		else if (!prefixcmp(a, "--export-pack-edges=")) {
>> +			char *filename = a + 20;
>> +
>>  			if (pack_edges)
>>  				fclose(pack_edges);
>> -			pack_edges = fopen(a + 20, "a");
>> +			pack_edges = fopen(filename, "a");
>>  			if (!pack_edges)
>> -				die("Cannot open %s: %s", a + 20, strerror(errno));
>> +				die("Cannot open %s: %s", filename, strerror(errno));
>>  		} else if (!strcmp(a, "--force"))
>>  			force_update = 1;
>>  		else if (!strcmp(a, "--quiet"))
>
> Normally, the compiler takes care of such optimizations. It actually
> takes care of it much better than you can do yourself, and doing it
> yourself can even sometimes generate less optimized code because it
> gets in the compiler optimizations'way.

True, but I think another point of the patch is to address the risk of two instances of "+ 20" going out of sync if/when the option parsing is updated.

Not that I think André meant the patch as defensive coding (the subject suggests it was meant to be a micro-optimization), nor this is the good way to address that risk factor (parse-options may be a better match for it).

André Goddard Rosa· Nov 25, 2007, 21:43 UTC · re: André Goddard Rosa · lore

[Resend PATCH] Avoid recalculating filename string pointer.

Hi, all!

   Avoid calculating string position in 2 different places.

From b6b05d9f8d8e053df4e971cd229e03b778c4d163 Mon Sep 17 00:00:00 2001
From: Andre Goddard Rosa <andre.goddard@gmail.com>
Date: Tue, 27 Nov 2007 10:17:54 -0200
Subject: [PATCH] Avoid recalculating filename string pointer.

Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com>
---
 fast-import.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Show changes to fast-import.c +4 −3

diff --git a/fast-import.c b/fast-import.c
index 98c2bd5..2d262eb 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2304,11 +2304,13 @@ int main(int argc, const char **argv)
                else if (!prefixcmp(a, "--export-marks="))
                        mark_file = a + 15;
                else if (!prefixcmp(a, "--export-pack-edges=")) {
+                       char *filename = a + 20;
+
                        if (pack_edges)
                                fclose(pack_edges);
-                       pack_edges = fopen(a + 20, "a");
+                       pack_edges = fopen(filename, "a");
                        if (!pack_edges)
-                               die("Cannot open %s: %s", a + 20,
strerror(errno));
+                               die("Cannot open %s: %s", filename,
strerror(errno));
                } else if (!strcmp(a, "--force"))
                        force_update = 1;
                else if (!strcmp(a, "--quiet"))
--
1.5.3.6.861.gd794-dirty

-- 
[]s,
André Goddard


From b6b05d9f8d8e053df4e971cd229e03b778c4d163 Mon Sep 17 00:00:00 2001
From: Andre Goddard Rosa <andre.goddard@gmail.com>
Date: Tue, 27 Nov 2007 10:17:54 -0200
Subject: [PATCH] Avoid recalculating filename string pointer.

Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com>
---
 fast-import.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 98c2bd5..2d262eb 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2304,11 +2304,13 @@ int main(int argc, const char **argv)
 		else if (!prefixcmp(a, "--export-marks="))
 			mark_file = a + 15;
 		else if (!prefixcmp(a, "--export-pack-edges=")) {
+			char *filename = a + 20;
+
 			if (pack_edges)
 				fclose(pack_edges);
-			pack_edges = fopen(a + 20, "a");
+			pack_edges = fopen(filename, "a");
 			if (!pack_edges)
-				die("Cannot open %s: %s", a + 20, strerror(errno));
+				die("Cannot open %s: %s", filename, strerror(errno));
 		} else if (!strcmp(a, "--force"))
 			force_update = 1;
 		else if (!strcmp(a, "--quiet"))
-- 
1.5.3.6.861.gd794-dirty