[PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings

[PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings

[Jeff King]

I was fooling around with clang and noticed that it complains about the
"int x = x" construct under -Wall. That is IMHO a deficiency in clang,
since the idiom has a well-defined use in silencing -Wuninitialized
warnings. But I've also always been nervous about the idiom, because
it's easy to get the analysis wrong (after all, the compiler gets these
cases wrong because they're complex), and it's possible for the code to
change later, introducing a new problem.

So I investigated our uses of the idiom. Many of them are correct and
still necessary. Some are correct but no longer necessary with modern
gcc. And some are technically correct, but the code and its assumptions
can be made clearer (to both a reader and the compiler) with a simple
rewrite. Patches are below for the latter two types.

Note that none of these fixes an actual bug in the current code; this is
purely maintenance hygiene. Nor do any of the patches depend on each
other; we can drop any of them that do not look they are providing a net
benefit.

  [1/4]: wt-status: fix possible use of uninitialized variable
  [2/4]: fast-import: use pointer-to-pointer to keep list tail
  [3/4]: drop some obsolete "x = x" compiler warning hacks
  [4/4]: transport: drop "int cmp = cmp" hack

-Peff

[PATCH 1/4] wt-status: fix possible use of uninitialized variable

[Jeff King]

In wt_status_print_change_data, we accept a change_type flag
that is meant to be either WT_STATUS_UPDATED or
WT_STATUS_CHANGED.  We then switch() on this value to set
the local variable "status" for each case, but do not
provide a fallback "default" label to the switch statement.

As a result, the compiler realizes that "status" might be
unset, and complains with a warning. To silence this
warning, we use the "int status = status" trick.  This is
correct with the current code, as all callers provide one of
the two expected change_type flags. However, it's also a
maintenance trap, as there is nothing to prevent future
callers from passing another flag, nor to document this
assumption.

Instead of using the "x = x" hack, let's handle the default
case in the switch() statement with a die("BUG"). That tells
the compiler and any readers of the code exactly what the
function's input assumptions are.

We could also convert the flag to an enum, which would
provide a compile-time check on the function input. However,
since these flags are part of a larger enum, that would make
the code unnecessarily complex (we would have to make a new
enum with just the two flags, and then convert it to the old
enum for passing to sub-functions).

Signed-off-by: Jeff King <peff@peff.net>
---
 wt-status.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index ef405d0..7555817 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -264,7 +264,7 @@ static void wt_status_print_change_data(struct wt_status *s,
 {
 	struct wt_status_change_data *d = it->util;
 	const char *c = color(change_type, s);
-	int status = status;
+	int status;
 	char *one_name;
 	char *two_name;
 	const char *one, *two;
@@ -292,6 +292,9 @@ static void wt_status_print_change_data(struct wt_status *s,
 		}
 		status = d->worktree_status;
 		break;
+	default:
+		die("BUG: unhandled change_type %d in wt_status_print_change_data",
+		    change_type);
 	}
 
 	one = quote_path(one_name, -1, &onebuf, s->prefix);
-- 
1.8.2.rc2.8.g2161951

Re: [PATCH 1/4] wt-status: fix possible use of uninitialized variable

[Jonathan Nieder]

Jeff King wrote:

> Instead of using the "x = x" hack, let's handle the default
> case in the switch() statement with a die("BUG"). That tells
> the compiler and any readers of the code exactly what the
> function's input assumptions are.

Sounds reasonable.

> We could also convert the flag to an enum, which would
> provide a compile-time check on the function input.

Unfortunately C permits out-of-bounds values for enums.

[...]
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -264,7 +264,7 @@ static void wt_status_print_change_data(struct wt_status *s,
>  {
>  	struct wt_status_change_data *d = it->util;
>  	const char *c = color(change_type, s);
> -	int status = status;
> +	int status;
>  	char *one_name;
>  	char *two_name;
>  	const char *one, *two;
> @@ -292,6 +292,9 @@ static void wt_status_print_change_data(struct wt_status *s,
>  		}
>  		status = d->worktree_status;
>  		break;
> +	default:
> +		die("BUG: unhandled change_type %d in wt_status_print_change_data",
> +		    change_type);

Micronit: s/unhandled/invalid/.

Thanks,
Jonathan

Re: [PATCH 1/4] wt-status: fix possible use of uninitialized variable

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jeff King wrote:
>
>> Instead of using the "x = x" hack, let's handle the default
>> case in the switch() statement with a die("BUG"). That tells
>> the compiler and any readers of the code exactly what the
>> function's input assumptions are.
>
> Sounds reasonable.
>
>> We could also convert the flag to an enum, which would
>> provide a compile-time check on the function input.
>
> Unfortunately C permits out-of-bounds values for enums.
>
> [...]
>> --- a/wt-status.c
>> +++ b/wt-status.c
>> @@ -264,7 +264,7 @@ static void wt_status_print_change_data(struct wt_status *s,
>>  {
>>  	struct wt_status_change_data *d = it->util;
>>  	const char *c = color(change_type, s);
>> -	int status = status;
>> +	int status;
>>  	char *one_name;
>>  	char *two_name;
>>  	const char *one, *two;
>> @@ -292,6 +292,9 @@ static void wt_status_print_change_data(struct wt_status *s,
>>  		}
>>  		status = d->worktree_status;
>>  		break;
>> +	default:
>> +		die("BUG: unhandled change_type %d in wt_status_print_change_data",
>> +		    change_type);
>
> Micronit: s/unhandled/invalid/.

I actually think "unhandled" is more correct for this one; we may
add new change_type later in the caller, and we do not want to
forget to add a new case arm that handles the new value.

Re: [PATCH 1/4] wt-status: fix possible use of uninitialized variable

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Jeff King wrote:

>>> +	default:
>>> +		die("BUG: unhandled change_type %d in wt_status_print_change_data",
>>> +		    change_type);
>>
>> Micronit: s/unhandled/invalid/.
>
> I actually think "unhandled" is more correct for this one; we may
> add new change_type later in the caller, and we do not want to
> forget to add a new case arm that handles the new value.

Ok.  Makes sense.

Re: [PATCH 1/4] wt-status: fix possible use of uninitialized variable

[Jeff King]

On Thu, Mar 21, 2013 at 12:49:50PM -0700, Jonathan Nieder wrote:

> > We could also convert the flag to an enum, which would
> > provide a compile-time check on the function input.
> 
> Unfortunately C permits out-of-bounds values for enums.

True, although I would think that most compilers take the hint for
switch() statements that handling all defined constants for an enum is
enough (certainly gcc does it with the "some enum constants not handled"
warning, but I did not actually check whether it does so in the
uninitialized-warning control flow checker).

Still, I'm happy enough with the die("BUG") that I posted, so we don't
need to worry about it.

-Peff

[PATCH 2/4] fast-import: use pointer-to-pointer to keep list tail

[Jeff King]

This is shorter, idiomatic, and it means the compiler does
not get confused about whether our "e" pointer is valid,
letting us drop the "e = e" hack.

Signed-off-by: Jeff King <peff@peff.net>
---
And it fixes an instance of Linus's "people do not understand pointers"
from here:

  http://meta.slashdot.org/story/12/10/11/0030249/linus-torvalds-answers-your-questions

 fast-import.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index c2a814e..583a439 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2613,7 +2613,7 @@ static struct hash_list *parse_merge(unsigned int *count)
 
 static struct hash_list *parse_merge(unsigned int *count)
 {
-	struct hash_list *list = NULL, *n, *e = e;
+	struct hash_list *list = NULL, **tail = &list, *n;
 	const char *from;
 	struct branch *s;
 
@@ -2641,11 +2641,9 @@ static struct hash_list *parse_merge(unsigned int *count)
 			die("Invalid ref name or SHA1 expression: %s", from);
 
 		n->next = NULL;
-		if (list)
-			e->next = n;
-		else
-			list = n;
-		e = n;
+		*tail = n;
+		tail = &n->next;
+
 		(*count)++;
 		read_next_command();
 	}
-- 
1.8.2.rc2.8.g2161951

Re: [PATCH 2/4] fast-import: use pointer-to-pointer to keep list tail

[Jonathan Nieder]

Jeff King wrote:

> This is shorter, idiomatic, and it means the compiler does
> not get confused about whether our "e" pointer is valid,
> letting us drop the "e = e" hack.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> And it fixes an instance of Linus's "people do not understand pointers"

Heh.  Yes, looks correct.  For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[PATCH 3/4] drop some obsolete "x = x" compiler warning hacks

[Jeff King]

In cases where the setting and access of a variable are
protected by the same conditional flag, older versions of
gcc would generate a "might be used unitialized" warning. We
silence the warning by initializing the variable to itself,
a hack that gcc recognizes.

Modern versions of gcc are smart enough to get this right,
going back to at least version 4.3.5. gcc 4.1 does get it
wrong in both cases, but is sufficiently old that we
probably don't need to care about it anymore.

Signed-off-by: Jeff King <peff@peff.net>
---
gcc 4.2 is conspicuously missing because no current Debian system even
has a backwards-compatibility package for it, making it harder to test.
And 4.3 was old enough for me to say "I do not care if you can run with
-Wall -Werror or not", let alone 4.2.

 builtin/cat-file.c | 2 +-
 fast-import.c      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 00528dd..ad29000 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -193,7 +193,7 @@ static int batch_one_object(const char *obj_name, int print_contents)
 	unsigned char sha1[20];
 	enum object_type type = 0;
 	unsigned long size;
-	void *contents = contents;
+	void *contents;
 
 	if (!obj_name)
 	   return 1;
diff --git a/fast-import.c b/fast-import.c
index 583a439..e12a8b8 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2434,7 +2434,7 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)
 {
 	const char *p = command_buf.buf + 2;
 	static struct strbuf uq = STRBUF_INIT;
-	struct object_entry *oe = oe;
+	struct object_entry *oe;
 	struct branch *s;
 	unsigned char sha1[20], commit_sha1[20];
 	char path[60];
-- 
1.8.2.rc2.8.g2161951

Re: [PATCH 3/4] drop some obsolete "x = x" compiler warning hacks

[Erik Faye-Lund]

On Thu, Mar 21, 2013 at 12:10 PM, Jeff King <peff@peff.net> wrote:
> In cases where the setting and access of a variable are
> protected by the same conditional flag, older versions of
> gcc would generate a "might be used unitialized" warning. We
> silence the warning by initializing the variable to itself,
> a hack that gcc recognizes.
>
> Modern versions of gcc are smart enough to get this right,
> going back to at least version 4.3.5. gcc 4.1 does get it
> wrong in both cases, but is sufficiently old that we
> probably don't need to care about it anymore.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> gcc 4.2 is conspicuously missing because no current Debian system even
> has a backwards-compatibility package for it, making it harder to test.
> And 4.3 was old enough for me to say "I do not care if you can run with
> -Wall -Werror or not", let alone 4.2.

Just a data-point. This is the version we use in msysGit:

$ gcc --version
gcc.exe (TDM-1 mingw32) 4.4.0

So yeah, it's not going to increase false positives here, I guess.

Re: [PATCH 3/4] drop some obsolete "x = x" compiler warning hacks

[Jonathan Nieder]

Jeff King wrote:

> And 4.3 was old enough for me to say "I do not care if you can run with
> -Wall -Werror or not", let alone 4.2.

Changes like this can only reveal bugs (in git or optimizers) that
were hidden before, without regressing actual runtime behavior, so for
what it's worth I like them.

I think perhaps we should encourage people to use
-Wno-error=uninitialized, in addition to cleaning up our code where
reasonably recent optimizers reveal it to be confusing.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 3/4] drop some obsolete "x = x" compiler warning hacks

[Torsten Bögershausen]

On 21.03.13 21:47, Jonathan Nieder wrote:
> Jeff King wrote:
> 
>> And 4.3 was old enough for me to say "I do not care if you can run with
>> -Wall -Werror or not", let alone 4.2.
> 
> Changes like this can only reveal bugs (in git or optimizers) that
> were hidden before, without regressing actual runtime behavior, so for
> what it's worth I like them.
> 
> I think perhaps we should encourage people to use
> -Wno-error=uninitialized, in addition to cleaning up our code where
> reasonably recent optimizers reveal it to be confusing.
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

I got 2 warnings, but reading the comments I feel that

Mac OS 10.6 and i686-apple-darwin10-gcc-4.2.1 (GCC) 4.2.1 (Apple Inc. build 5666) (dot 3)

is outdated ;-)


builtin/cat-file.c: In function ~cmd_cat_file~:
builtin/cat-file.c:196: warning: ~contents~ may be used uninitialized in this function
builtin/cat-file.c:196: note: ~contents~ was declared here


fast-import.c: In function ‘parse_new_commit’:
fast-import.c:2438: warning: ‘oe’ may be used uninitialized in this function
fast-import.c:2438: note: ‘oe’ was declared here

[PATCH 4/4] transport: drop "int cmp = cmp" hack

[Jeff King]

According to 47ec794, this initialization is meant to
squelch an erroneous uninitialized variable warning from gcc
4.0.1.  That version is quite old at this point, and gcc 4.1
and up handle it fine, with one exception. There seems to be
a regression in gcc 4.6.3, which produces the warning;
however, gcc versions 4.4.7 and 4.7.2 do not.

Signed-off-by: Jeff King <peff@peff.net>
---
We probably _don't_ want to apply this one right now. The regression in
4.6 means some people on reasonably modern systems probably would still
see the warning. Debian stable ships with 4.4, and testing/unstable
defaults to 4.7 (though you can install a gcc-4.6 compatibility
package). But I have no clue if other distros made releases with 4.6.

 transport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/transport.c b/transport.c
index 886ffd8..87b8f14 100644
--- a/transport.c
+++ b/transport.c
@@ -106,7 +106,7 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
 		return;
 
 	for (;;) {
-		int cmp = cmp, len;
+		int cmp, len;
 
 		if (!fgets(buffer, sizeof(buffer), f)) {
 			fclose(f);
-- 
1.8.2.rc2.8.g2161951

Re: [PATCH 4/4] transport: drop "int cmp = cmp" hack

[Jonathan Nieder]

Jeff King wrote:

> We probably _don't_ want to apply this one right now.

I think we should.  gcc 4.6.y warning bugs should be fixed --- there's
no need for git to work around them.  And anyone affected can easily
stop using -Werror (-Werror is not meant for use by non-developers in
production).

So fwiw
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 4/4] transport: drop "int cmp = cmp" hack

[Junio C Hamano]

On Thu, Mar 21, 2013 at 4:13 AM, Jeff King <peff@peff.net> wrote:
>
> According to 47ec794, this initialization is meant to
> squelch an erroneous uninitialized variable warning from gcc
> 4.0.1.  That version is quite old at this point, and gcc 4.1
> and up handle it fine, with one exception. There seems to be
> a regression in gcc 4.6.3, which produces the warning;
> however, gcc versions 4.4.7 and 4.7.2 do not.
>

transport.c: In function 'get_refs_via_rsync':
transport.c:127:29: error: 'cmp' may be used uninitialized in this
function [-Werror=uninitialized]
transport.c:109:7: note: 'cmp' was declared here

gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3


Sigh...

Re: [PATCH 4/4] transport: drop "int cmp = cmp" hack

[Jeff King]

On Sat, Mar 23, 2013 at 09:00:05PM -0700, Junio C Hamano wrote:

> On Thu, Mar 21, 2013 at 4:13 AM, Jeff King <peff@peff.net> wrote:
> >
> > According to 47ec794, this initialization is meant to
> > squelch an erroneous uninitialized variable warning from gcc
> > 4.0.1.  That version is quite old at this point, and gcc 4.1
> > and up handle it fine, with one exception. There seems to be
> > a regression in gcc 4.6.3, which produces the warning;
> > however, gcc versions 4.4.7 and 4.7.2 do not.
> >
> 
> transport.c: In function 'get_refs_via_rsync':
> transport.c:127:29: error: 'cmp' may be used uninitialized in this
> function [-Werror=uninitialized]
> transport.c:109:7: note: 'cmp' was declared here
> 
> gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3

Right, that's the same version I noted above. Is 4.6.3 the default
compiler under a particular release of Ubuntu, or did you use their
gcc-4.6 package?

-Peff

Re: [PATCH 4/4] transport: drop "int cmp = cmp" hack

[Torsten Bögershausen]

On 24.03.13 10:32, Jeff King wrote:
> On Sat, Mar 23, 2013 at 09:00:05PM -0700, Junio C Hamano wrote:
> 
>> On Thu, Mar 21, 2013 at 4:13 AM, Jeff King <peff@peff.net> wrote:
>>>
>>> According to 47ec794, this initialization is meant to
>>> squelch an erroneous uninitialized variable warning from gcc
>>> 4.0.1.  That version is quite old at this point, and gcc 4.1
>>> and up handle it fine, with one exception. There seems to be
>>> a regression in gcc 4.6.3, which produces the warning;
>>> however, gcc versions 4.4.7 and 4.7.2 do not.
>>>
>>
>> transport.c: In function 'get_refs_via_rsync':
>> transport.c:127:29: error: 'cmp' may be used uninitialized in this
>> function [-Werror=uninitialized]
>> transport.c:109:7: note: 'cmp' was declared here
>>
>> gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
> 
> Right, that's the same version I noted above. Is 4.6.3 the default
> compiler under a particular release of Ubuntu, or did you use their
> gcc-4.6 package?
> 
> -Peff
Side question:
How much does it hurt to write like this:

diff --git a/transport.c b/transport.c
index 6b2ae94..8020b62 100644
--- a/transport.c
+++ b/transport.c
@@ -106,7 +106,7 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
                return;
 
        for (;;) {
-               int cmp, len;
+               int cmp=0, len;
 
==============

Using Ubuntu 10.4, using gcc (Ubuntu 4.4.3-4ubuntu5.1) 4.4.3
the compiler will add a line like this:

  2e83:	31 ff                	xor    %edi,%edi

(Which should not be to slow to execute)

Looking at a later gcc, from upcoming Debian, with gcc (Debian 4.7.2-5) 4.7.2
the assembly code is exactly the same ;-)

Re: [PATCH 4/4] transport: drop "int cmp = cmp" hack

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Mar 23, 2013 at 09:00:05PM -0700, Junio C Hamano wrote:
>
>> On Thu, Mar 21, 2013 at 4:13 AM, Jeff King <peff@peff.net> wrote:
>> >
>> > According to 47ec794, this initialization is meant to
>> > squelch an erroneous uninitialized variable warning from gcc
>> > 4.0.1.  That version is quite old at this point, and gcc 4.1
>> > and up handle it fine, with one exception. There seems to be
>> > a regression in gcc 4.6.3, which produces the warning;
>> > however, gcc versions 4.4.7 and 4.7.2 do not.
>> >
>> 
>> transport.c: In function 'get_refs_via_rsync':
>> transport.c:127:29: error: 'cmp' may be used uninitialized in this
>> function [-Werror=uninitialized]
>> transport.c:109:7: note: 'cmp' was declared here
>> 
>> gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
>
> Right, that's the same version I noted above. Is 4.6.3 the default
> compiler under a particular release of Ubuntu, or did you use their
> gcc-4.6 package?

I'll check later with one of my VMs.  The copy of U 12.04 I happened
to have handy has that version installed.

By the way, I find this piece of code less than pleasant:

 * It uses "struct ref dummy = { NULL }, *tail = &dummy", and then
   accumulates things by appending to "&tail" and then returns
   dummy.next.  Why doesn't it do

	struct ref *retval = NULL, **tail = &retval;

   and pass tail around to append things, like everybody else?  Is
   this another instance of "People do not understand linked list"
   problem?  Perhaps fixing that may unconfuse the compiler?

 * Its read_loose_refs() is a recursive function that sorts the
   results from readdir(3) and iterates over them, expecting its
   recursive call to fail _only_ when the entry it read is not a
   directory that it needs to recurse into.

   It is not obvious if the resulting list is sorted correctly with
   this loop structure when you have branches "foo.bar", "foo/bar",
   and "foo=bar".  I think the loop first reads "foo", "foo.bar" and
   "foo=bar", sorts them in that order, and starts reading
   recursively, ending up with "foo/bar" first and then "foo.bar"
   and finally "foo=bar".  Later, the tail of the same list is
   passed to insert_packed_refs(), which does in-place merging of
   this list and the contents of the packed_refs file.  These two
   data sources have to be sorted the same way for this merge to
   work correctly, but there is no validating the order of the
   entries it reads from the packed-refs file.  At least, it should
   barf when the file is not sorted.  It could be lenient and accept
   a mal-sorted input, but I do not think that is advisable.

I'll apply the attached on 'maint' for now, as rsync is not worth
spending too many cycles on worrying about; I need to go to the
bathroom to wash my eyes after staring this code for 20 minutes X-<.

 transport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/transport.c b/transport.c
index 87b8f14..e6f9346 100644
--- a/transport.c
+++ b/transport.c
@@ -106,7 +106,8 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
 		return;
 
 	for (;;) {
-		int cmp, len;
+		int cmp = 0; /* assigned before used */
+		int len;
 
 		if (!fgets(buffer, sizeof(buffer), f)) {
 			fclose(f);

Re: [PATCH 4/4] transport: drop "int cmp = cmp" hack

[Jeff King]

On Mon, Mar 25, 2013 at 12:50:54PM -0700, Junio C Hamano wrote:

> >> transport.c: In function 'get_refs_via_rsync':
> >> transport.c:127:29: error: 'cmp' may be used uninitialized in this
> >> function [-Werror=uninitialized]
> >> transport.c:109:7: note: 'cmp' was declared here
> >> 
> >> gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
> >
> > Right, that's the same version I noted above. Is 4.6.3 the default
> > compiler under a particular release of Ubuntu, or did you use their
> > gcc-4.6 package?
> 
> I'll check later with one of my VMs.  The copy of U 12.04 I happened
> to have handy has that version installed.

Ah, if you didn't explicitly run "gcc-4.6", then it was probably the
default version in 12.04 (as it was for a while in Debian testing, but
they never actually made a release with it, so everybody is now on 4.7
by default).

> By the way, I find this piece of code less than pleasant:
> 
>  * It uses "struct ref dummy = { NULL }, *tail = &dummy", and then
>    accumulates things by appending to "&tail" and then returns
>    dummy.next.  Why doesn't it do
> 
> 	struct ref *retval = NULL, **tail = &retval;
> 
>    and pass tail around to append things, like everybody else?  Is
>    this another instance of "People do not understand linked list"
>    problem?  Perhaps fixing that may unconfuse the compiler?

Ugh, that is horrible. At first I thought it was even wrong, as we pass
&tail and not &dummy.next to read_loose_refs. But two wrongs _do_ make a
right, because read_loose_refs, rather than do:

  *tail = new;
  tail = &new->next;

does:

  (*tail)->next = new;
  *tail = new;

>    Later, the tail of the same list is passed to insert_packed_refs(),
>    which does in-place merging of this list and the contents of the
>    packed_refs file.  These two data sources have to be sorted the
>    same way for this merge to work correctly, but there is no
>    validating the order of the entries it reads from the packed-refs
>    file.  At least, it should barf when the file is not sorted.  It
>    could be lenient and accept a mal-sorted input, but I do not think
>    that is advisable.

Actually, it is the head of the loose list (though it is hard to
realize, because it is called tail!).

> I'll apply the attached on 'maint' for now, as rsync is not worth
> spending too many cycles on worrying about; I need to go to the
> bathroom to wash my eyes after staring this code for 20 minutes X-<.

Yeah, it's quite ugly. I really wonder if it is time to drop rsync
support. I'd be really surprised if anybody is actively using it.

I wonder, though, what made you look at this. It did not come up in my
list of -Wuninitialized warnings. Did it get triggered by one of the
other gcc versions?

> diff --git a/transport.c b/transport.c
> index 87b8f14..e6f9346 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -106,7 +106,8 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
>  		return;
>  
>  	for (;;) {
> -		int cmp, len;
> +		int cmp = 0; /* assigned before used */
> +		int len;
>  
>  		if (!fgets(buffer, sizeof(buffer), f)) {
>  			fclose(f);

I think that's fine.

-Peff

Re: [PATCH 4/4] transport: drop "int cmp = cmp" hack

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I wonder, though, what made you look at this. It did not come up in my
> list of -Wuninitialized warnings. Did it get triggered by one of the
> other gcc versions?

No, but the function in question has that questionable construct
written by somebody who does not understand linked list, and it
dusgusted me enough to look at where that list came from, which
inevitably made me notice that "return dummy.next" that made me go
"wat?"

>
>> diff --git a/transport.c b/transport.c
>> index 87b8f14..e6f9346 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -106,7 +106,8 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
>>  		return;
>>  
>>  	for (;;) {
>> -		int cmp, len;
>> +		int cmp = 0; /* assigned before used */
>> +		int len;
>>  
>>  		if (!fgets(buffer, sizeof(buffer), f)) {
>>  			fclose(f);
>
> I think that's fine.
>
> -Peff

Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings

[Johannes Sixt]

Am 3/21/2013 12:03, schrieb Jeff King:
> I was fooling around with clang and noticed that it complains about the
> "int x = x" construct under -Wall. That is IMHO a deficiency in clang,
> since the idiom has a well-defined use in silencing -Wuninitialized
> warnings.

IMO, that's a myth. The construct invokes undefined behavior at least
since C99, and the compilers are right to complain about it.

But you might just say that standards are not worth the paper they are
printed on, and you may possibly be right for practical reasons. But I
still consider it a myth that "int x = x" is an idiom. I'm in the C
business since more than 25 years, and the first time I saw the "idiom"
was in git code. Is there any evidence that the construct is used
elsewhere? Have I been in the wrong corner of the C world for such a long
time?

-- Hannes

Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings

[Jeff King]

On Thu, Mar 21, 2013 at 12:45:37PM +0100, Johannes Sixt wrote:

> Am 3/21/2013 12:03, schrieb Jeff King:
> > I was fooling around with clang and noticed that it complains about the
> > "int x = x" construct under -Wall. That is IMHO a deficiency in clang,
> > since the idiom has a well-defined use in silencing -Wuninitialized
> > warnings.
> 
> IMO, that's a myth. The construct invokes undefined behavior at least
> since C99, and the compilers are right to complain about it.

While undefined behavior does leave the compiler free to do anything,
including nasal demons, it would be a very poor implementation that did
anything except leave random bytes in the value. And it also means that
gcc is free to take it as a hint to silence the warning; given that
clang tries to be compatible with gcc, I'd think it would want to do the
same. But I may be wrong that the behavior from gcc is intentional or
common (see below).

> But you might just say that standards are not worth the paper they are
> printed on, and you may possibly be right for practical reasons. But I
> still consider it a myth that "int x = x" is an idiom. I'm in the C
> business since more than 25 years, and the first time I saw the "idiom"
> was in git code. Is there any evidence that the construct is used
> elsewhere? Have I been in the wrong corner of the C world for such a long
> time?

Git code was my introduction to it, too, and I was led to believe it was
idiomatic, so I can't speak further on that. I think it was Junio who
introduced me to it, so maybe he can shed more light on the history.

-Peff

Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Git code was my introduction to it, too, and I was led to believe it was
> idiomatic, so I can't speak further on that. I think it was Junio who
> introduced me to it, so maybe he can shed more light on the history.

I think we picked the convention up from the kernel folks.  At least
that is how I first met the construct.  The uninitialized_var(x)
macro was (and still is) used to mark these "The compiler is too
dumb to realize, but we know what we are doing" cases:

    $ git grep '#define uninitialized_var' include/
    include/linux/compiler-gcc.h:#define uninitialized_var(x) x = x
    include/linux/compiler-intel.h:#define uninitialized_var(x) x

but they recently had a discussion, e.g.

    http://thread.gmane.org/gmane.linux.kernel.openipmi/1998/focus=1383705

so...

Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> Git code was my introduction to it, too, and I was led to believe it was
>> idiomatic, so I can't speak further on that. I think it was Junio who
>> introduced me to it, so maybe he can shed more light on the history.
>
> I think we picked the convention up from the kernel folks.  At least
> that is how I first met the construct.  The uninitialized_var(x)
> macro was (and still is) used to mark these "The compiler is too
> dumb to realize, but we know what we are doing" cases:
>
>     $ git grep '#define uninitialized_var' include/
>     include/linux/compiler-gcc.h:#define uninitialized_var(x) x = x
>     include/linux/compiler-intel.h:#define uninitialized_var(x) x
>
> but they recently had a discussion, e.g.
>
>     http://thread.gmane.org/gmane.linux.kernel.openipmi/1998/focus=1383705
>
> so...

While flipping the paragraphs around before sending the message out
I managed to lose the important one.  Here is roughly what I wrote:

    I am for dropping "= x" and leaving it uninitialized at the
    declaration site, or explicitly initializing it to some
    reasonable starting value (e.g. NULL if it is a pointer) and
    adding a comment to say that the initialization is to squelch
    compiler warnings.

Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings

[Jeff King]

On Thu, Mar 21, 2013 at 08:19:43AM -0700, Junio C Hamano wrote:

> >     $ git grep '#define uninitialized_var' include/
> >     include/linux/compiler-gcc.h:#define uninitialized_var(x) x = x
> >     include/linux/compiler-intel.h:#define uninitialized_var(x) x
> >
> > but they recently had a discussion, e.g.
> >
> >     http://thread.gmane.org/gmane.linux.kernel.openipmi/1998/focus=1383705
> >
> > so...
> 
> While flipping the paragraphs around before sending the message out
> I managed to lose the important one.  Here is roughly what I wrote:
> 
>     I am for dropping "= x" and leaving it uninitialized at the
>     declaration site, or explicitly initializing it to some
>     reasonable starting value (e.g. NULL if it is a pointer) and
>     adding a comment to say that the initialization is to squelch
>     compiler warnings.

I'd be in favor of that, too. In many cases, I think the fact that gcc
cannot trace the control flow is a good indication that it is hard for a
human to trace it, too. And in those cases we would be better off
restructuring the code slightly to make it more obvious to both types of
readers.

Two patches to follow.

  [5/4]: fast-import: clarify "inline" logic in file_change_m
  [6/4]: run-command: always set failed_errno in start_command

-Peff

[PATCH 5/4] fast-import: clarify "inline" logic in file_change_m

[Jeff King]

When we read a fast-import line like:

  M 100644 :1 foo.c

we point the local object_entry variable "oe" to the object
named by the mark ":1". When the input uses the "inline"
construct, however, we do not have such an object_entry.

The current code is careful not to access "oe" in the inline
case, but we can make the assumption even more obvious (and
catch violations of it) by setting oe to NULL and adding a
comment. As a bonus, this also squelches an over-zealous gcc
-Wuninitialized warning, which means we can drop the "oe =
oe" initialization hack.

Signed-off-by: Jeff King <peff@peff.net>
---
 fast-import.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fast-import.c b/fast-import.c
index e12a8b8..a0c2c2f 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2265,7 +2265,7 @@ static void file_change_m(struct branch *b)
 	const char *p = command_buf.buf + 2;
 	static struct strbuf uq = STRBUF_INIT;
 	const char *endp;
-	struct object_entry *oe = oe;
+	struct object_entry *oe;
 	unsigned char sha1[20];
 	uint16_t mode, inline_data = 0;
 
@@ -2292,6 +2292,7 @@ static void file_change_m(struct branch *b)
 		hashcpy(sha1, oe->idx.sha1);
 	} else if (!prefixcmp(p, "inline ")) {
 		inline_data = 1;
+		oe = NULL; /* not used with inline_data, but makes gcc happy */
 		p += strlen("inline");  /* advance to space */
 	} else {
 		if (get_sha1_hex(p, sha1))
-- 
1.8.2.rc2.8.g2161951

[PATCH 6/4] run-command: always set failed_errno in start_command

[Jeff King]

When we fail to fork, we set the failed_errno variable to
the value of errno so it is not clobbered by later syscalls.
However, we do so in a conditional, and it is hard to see
later under what conditions the variable has a valid value.

Instead of setting it only when fork fails, let's just
always set it after forking. This is more obvious for human
readers (as we are no longer setting it as a side effect of
a strerror call), and it is more obvious to gcc, which no
longer generates a spurious -Wuninitialized warning. It also
happens to match what the WIN32 half of the #ifdef does.

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/run-command.c b/run-command.c
index 07e27ff..765c2ce 100644
--- a/run-command.c
+++ b/run-command.c
@@ -273,7 +273,7 @@ int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
 	int fdin[2], fdout[2], fderr[2];
-	int failed_errno = failed_errno;
+	int failed_errno;
 	char *str;
 
 	/*
@@ -341,6 +341,7 @@ fail_pipe:
 		notify_pipe[0] = notify_pipe[1] = -1;
 
 	cmd->pid = fork();
+	failed_errno = errno;
 	if (!cmd->pid) {
 		/*
 		 * Redirect the channel to write syscall error messages to
@@ -420,7 +421,7 @@ fail_pipe:
 	}
 	if (cmd->pid < 0)
 		error("cannot fork() for %s: %s", cmd->argv[0],
-			strerror(failed_errno = errno));
+			strerror(errno));
 	else if (cmd->clean_on_exit)
 		mark_child_for_cleanup(cmd->pid);
 
-- 
1.8.2.rc2.8.g2161951

Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings

[Jonathan Nieder]

Jeff King wrote:

> Two patches to follow.
>
>   [5/4]: fast-import: clarify "inline" logic in file_change_m

This one is clearly a bug / missing feature in gcc's control flow
analysis, but your workaround looks reasonable.

>   [6/4]: run-command: always set failed_errno in start_command

Very sane.  Thanks.

Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings

[Jeff King]

On Thu, Mar 21, 2013 at 11:44:02AM -0400, Jeff King wrote:

> >     I am for dropping "= x" and leaving it uninitialized at the
> >     declaration site, or explicitly initializing it to some
> >     reasonable starting value (e.g. NULL if it is a pointer) and
> >     adding a comment to say that the initialization is to squelch
> >     compiler warnings.
> 
> I'd be in favor of that, too. In many cases, I think the fact that gcc
> cannot trace the control flow is a good indication that it is hard for a
> human to trace it, too. And in those cases we would be better off
> restructuring the code slightly to make it more obvious to both types of
> readers.
> 
> Two patches to follow.
> 
>   [5/4]: fast-import: clarify "inline" logic in file_change_m
>   [6/4]: run-command: always set failed_errno in start_command

And here are two more; with these, our code base should be free of "x =
x" initializations (at least according to clang).

  [7/4]: submodule: clarify logic in show_submodule_summary
  [8/4]: match-trees: drop "x = x" initializations

Not pressing, obviously, but since I had just analyzed the code
yesterday, I wanted to do it while they were still fresh in my mind.

-Peff

[PATCH 7/4] submodule: clarify logic in show_submodule_summary

[Jeff King]

There are two uses of the "left" and "right" commit
variables that make it hard to be sure what values they
have (both for the reader, and for gcc, which wrongly
complains that they might be used uninitialized).

The functions starts with a cascading if statement, checking
that the input sha1s exist, and finally working up to
preparing a revision walk. We only prepare the walk if the
cascading conditional did not find any problems, which we
check by seeing whether it set the "message" variable or
not. It's simpler and more obvious to just add a condition
to the end of the cascade.

Later, we check the same "message" variable when deciding
whether to clear commit marks on the left/right commits; if
it is set, we presumably never started the walk. This is
wrong, though; we might have started the walk and munged
commit flags, only to encounter an error afterwards. We
should always clear the flags on left/right if they exist,
whether the walk was successful or not.

Signed-off-by: Jeff King <peff@peff.net>
---
 submodule.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/submodule.c b/submodule.c
index 9ba1496..975bc87 100644
--- a/submodule.c
+++ b/submodule.c
@@ -261,7 +261,7 @@ void show_submodule_summary(FILE *f, const char *path,
 		const char *del, const char *add, const char *reset)
 {
 	struct rev_info rev;
-	struct commit *left = left, *right = right;
+	struct commit *left = NULL, *right = NULL;
 	const char *message = NULL;
 	struct strbuf sb = STRBUF_INIT;
 	int fast_forward = 0, fast_backward = 0;
@@ -275,10 +275,8 @@ void show_submodule_summary(FILE *f, const char *path,
 	else if (!(left = lookup_commit_reference(one)) ||
 		 !(right = lookup_commit_reference(two)))
 		message = "(commits not present)";
-
-	if (!message &&
-	    prepare_submodule_summary(&rev, path, left, right,
-					&fast_forward, &fast_backward))
+	else if (prepare_submodule_summary(&rev, path, left, right,
+					   &fast_forward, &fast_backward))
 		message = "(revision walker failed)";
 
 	if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
@@ -302,11 +300,12 @@ void show_submodule_summary(FILE *f, const char *path,
 		strbuf_addf(&sb, "%s:%s\n", fast_backward ? " (rewind)" : "", reset);
 	fwrite(sb.buf, sb.len, 1, f);
 
-	if (!message) {
+	if (!message) /* only NULL if we succeeded in setting up the walk */
 		print_submodule_summary(&rev, f, del, add, reset);
+	if (left)
 		clear_commit_marks(left, ~0);
+	if (right)
 		clear_commit_marks(right, ~0);
-	}
 
 	strbuf_release(&sb);
 }
-- 
1.8.2.13.g0f18d3c

Re: [PATCH 7/4] submodule: clarify logic in show_submodule_summary

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> There are two uses of the "left" and "right" commit
> variables that make it hard to be sure what values they
> have (both for the reader, and for gcc, which wrongly
> complains that they might be used uninitialized).
>
> The functions starts with a cascading if statement, checking
> that the input sha1s exist, and finally working up to
> preparing a revision walk. We only prepare the walk if the
> cascading conditional did not find any problems, which we
> check by seeing whether it set the "message" variable or
> not. It's simpler and more obvious to just add a condition
> to the end of the cascade.
>
> Later, we check the same "message" variable when deciding
> whether to clear commit marks on the left/right commits; if
> it is set, we presumably never started the walk. This is
> wrong, though; we might have started the walk and munged
> commit flags, only to encounter an error afterwards. We
> should always clear the flags on left/right if they exist,
> whether the walk was successful or not.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

Looks good.  Thanks.

>  submodule.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 9ba1496..975bc87 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -261,7 +261,7 @@ void show_submodule_summary(FILE *f, const char *path,
>  		const char *del, const char *add, const char *reset)
>  {
>  	struct rev_info rev;
> -	struct commit *left = left, *right = right;
> +	struct commit *left = NULL, *right = NULL;
>  	const char *message = NULL;
>  	struct strbuf sb = STRBUF_INIT;
>  	int fast_forward = 0, fast_backward = 0;
> @@ -275,10 +275,8 @@ void show_submodule_summary(FILE *f, const char *path,
>  	else if (!(left = lookup_commit_reference(one)) ||
>  		 !(right = lookup_commit_reference(two)))
>  		message = "(commits not present)";
> -
> -	if (!message &&
> -	    prepare_submodule_summary(&rev, path, left, right,
> -					&fast_forward, &fast_backward))
> +	else if (prepare_submodule_summary(&rev, path, left, right,
> +					   &fast_forward, &fast_backward))
>  		message = "(revision walker failed)";
>  
>  	if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
> @@ -302,11 +300,12 @@ void show_submodule_summary(FILE *f, const char *path,
>  		strbuf_addf(&sb, "%s:%s\n", fast_backward ? " (rewind)" : "", reset);
>  	fwrite(sb.buf, sb.len, 1, f);
>  
> -	if (!message) {
> +	if (!message) /* only NULL if we succeeded in setting up the walk */
>  		print_submodule_summary(&rev, f, del, add, reset);
> +	if (left)
>  		clear_commit_marks(left, ~0);
> +	if (right)
>  		clear_commit_marks(right, ~0);
> -	}
>  
>  	strbuf_release(&sb);
>  }

[PATCH 8/4] match-trees: drop "x = x" initializations

[Jeff King]

These nonsense assignments are meant to squelch gcc warnings
that the variables might be used uninitialized. However, gcc
gets it mostly right, realizing that we will either
extract tree entries from both sides, or we will hit a
"continue" statement and go to the top of the loop.

However, while getting this right for the "elem" and "path"
variables, it does not do so for the "mode" variables. Let's
drop the nonsense initialization where modern gcc does not
need them, and just set the modes to "0", along with a
comment. These values should never be used, but it makes
both gcc, as well as any compiler which does not like the "x
= x" initializations, happy.

While we're in the area, let's also update the loop
condition to use logical-OR rather than bitwise-OR. They should
be equivalent in this case, and the use of the latter was
probably a typo.

Signed-off-by: Jeff King <peff@peff.net>
---
Of the 8 patches, this is the one I find the least satisfying, if only
because I do not think gcc's failure is because of complicated control
flow, and rearranging the code would only hurt readability. And I'm
quite curious why it complains about "mode", but not about the other
variables, which are set in the exact same place (and why it would not
be able to handle such a simple control flow at all).

It makes me wonder if I am missing something, or there is some subtle
bug. But I can't see it. Other eyes appreciated.

 match-trees.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/match-trees.c b/match-trees.c
index 26f7ed1..4360f10 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -71,13 +71,13 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)
 	if (type != OBJ_TREE)
 		die("%s is not a tree", sha1_to_hex(hash2));
 	init_tree_desc(&two, two_buf, size);
-	while (one.size | two.size) {
-		const unsigned char *elem1 = elem1;
-		const unsigned char *elem2 = elem2;
-		const char *path1 = path1;
-		const char *path2 = path2;
-		unsigned mode1 = mode1;
-		unsigned mode2 = mode2;
+	while (one.size || two.size) {
+		const unsigned char *elem1;
+		const unsigned char *elem2;
+		const char *path1;
+		const char *path2;
+		unsigned mode1 = 0; /* make gcc happy */
+		unsigned mode2 = 0; /* make gcc happy */
 		int cmp;
 
 		if (one.size)
-- 
1.8.2.13.g0f18d3c

Re: [PATCH 8/4] match-trees: drop "x = x" initializations

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Of the 8 patches, this is the one I find the least satisfying, if only
> because I do not think gcc's failure is because of complicated control
> flow, and rearranging the code would only hurt readability. And I'm
> quite curious why it complains about "mode", but not about the other
> variables, which are set in the exact same place (and why it would not
> be able to handle such a simple control flow at all).
>
> It makes me wonder if I am missing something, or there is some subtle
> bug. But I can't see it. Other eyes appreciated.

I obviously am not qualified as "other eyes" to catch bugs in this
code as this is entirely mine, but I do not see any obvious reason
that would make the compiler to think mode[12] less initialized than
elem[12] or path[12] either.

These three are all updated by the same tree_entry_extract() call,
and whenever we use mode[12] we use path[12], so if it decides path1
is used or assigned, it should be able to tell mode1 is, too.

Unsatisfactory, it surely is...

>  match-trees.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/match-trees.c b/match-trees.c
> index 26f7ed1..4360f10 100644
> --- a/match-trees.c
> +++ b/match-trees.c
> @@ -71,13 +71,13 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)
>  	if (type != OBJ_TREE)
>  		die("%s is not a tree", sha1_to_hex(hash2));
>  	init_tree_desc(&two, two_buf, size);
> -	while (one.size | two.size) {
> -		const unsigned char *elem1 = elem1;
> -		const unsigned char *elem2 = elem2;
> -		const char *path1 = path1;
> -		const char *path2 = path2;
> -		unsigned mode1 = mode1;
> -		unsigned mode2 = mode2;
> +	while (one.size || two.size) {
> +		const unsigned char *elem1;
> +		const unsigned char *elem2;
> +		const char *path1;
> +		const char *path2;
> +		unsigned mode1 = 0; /* make gcc happy */
> +		unsigned mode2 = 0; /* make gcc happy */
>  		int cmp;
>  
>  		if (one.size)

Re: [PATCH 8/4] match-trees: drop "x = x" initializations

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> These three are all updated by the same tree_entry_extract() call,
> and whenever we use mode[12] we use path[12], so if it decides path1
> is used or assigned, it should be able to tell mode1 is, too.
>
> Unsatisfactory, it surely is...

And immediately after I wrote the above, I am greeted by this:

    gcc (Debian 4.4.5-8) 4.4.5
    match-trees.c:75: error: 'elem1' may be used uninitialized in this function
    match-trees.c:77: error: 'path1' may be used uninitialized in this function

and this crazy one on top squelches it.

If you flip the order of four lines that extracts only when size is
non-zero to extract first from two into elem2, then the warning is
given for elem2/path2 but not for elem1/path1.

I'll initialize all of them to nonsense values for now.

diff --git a/match-trees.c b/match-trees.c
index 4360f10..88981e8 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -72,9 +72,9 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)
 		die("%s is not a tree", sha1_to_hex(hash2));
 	init_tree_desc(&two, two_buf, size);
 	while (one.size || two.size) {
-		const unsigned char *elem1;
+		const unsigned char *elem1 = NULL;
 		const unsigned char *elem2;
-		const char *path1;
+		const char *path1 = NULL;
 		const char *path2;
 		unsigned mode1 = 0; /* make gcc happy */
 		unsigned mode2 = 0; /* make gcc happy */

Re: [PATCH 8/4] match-trees: drop "x = x" initializations

[Jeff King]

On Fri, Mar 22, 2013 at 02:33:16PM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > These three are all updated by the same tree_entry_extract() call,
> > and whenever we use mode[12] we use path[12], so if it decides path1
> > is used or assigned, it should be able to tell mode1 is, too.
> >
> > Unsatisfactory, it surely is...
> 
> And immediately after I wrote the above, I am greeted by this:
> 
>     gcc (Debian 4.4.5-8) 4.4.5
>     match-trees.c:75: error: 'elem1' may be used uninitialized in this function
>     match-trees.c:77: error: 'path1' may be used uninitialized in this function
> 
> and this crazy one on top squelches it.

Ugh, yeah, I should have tried with more compilers. 4.6 complains, but
4.7 doesn't (although I still find it really weird that 4.7 gets it
_half_ right).

> I'll initialize all of them to nonsense values for now.

I think that's sensible.

-Peff

Re: [PATCH 8/4] match-trees: drop "x = x" initializations

[René Scharfe]

Am 22.03.2013 17:21, schrieb Jeff King:
> Of the 8 patches, this is the one I find the least satisfying, if only
> because I do not think gcc's failure is because of complicated control
> flow, and rearranging the code would only hurt readability.

Hmm, let's see if we can help the compiler follow the code without
making it harder for people to understand.  The patch looks a bit
jumbled, but the resulting code is OK in my biased opinion.

-- >8 --
There are two ways we can spot missing entries, i.e. added or removed
files: By reaching the end of one of the trees while the other still
has entries, or in the middle of the two lists with base_name_compare().
Missing files are handled the same in either case, but the code is
duplicated.

Unify the handling by just setting cmp appropriately when running off
a tree instead of handling the case on the spot.  If both trees contain
entries, call base_name_compare() as usual.

This make the code slightly shorter, and also helps gcc 4.6 to
understand that none of the variables in the loop are used without
initialization.  Therefore we can remove the trick to initialize them
using themselves, which was used to squelch false warnings.

[Stolen from Jeff King:]
While we're in the area, let's also update the loop
condition to use logical-OR rather than bitwise-OR. They should
be equivalent in this case, and the use of the latter was
probably a typo.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 match-trees.c | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/match-trees.c b/match-trees.c
index 26f7ed1..c0c66bb 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -71,34 +71,26 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)
 	if (type != OBJ_TREE)
 		die("%s is not a tree", sha1_to_hex(hash2));
 	init_tree_desc(&two, two_buf, size);
-	while (one.size | two.size) {
-		const unsigned char *elem1 = elem1;
-		const unsigned char *elem2 = elem2;
-		const char *path1 = path1;
-		const char *path2 = path2;
-		unsigned mode1 = mode1;
-		unsigned mode2 = mode2;
-		int cmp;
+	while (one.size || two.size) {
+		const unsigned char *elem1, *elem2;
+		const char *path1, *path2;
+		unsigned mode1, mode2;
+		int cmp = 0;
 
 		if (one.size)
 			elem1 = tree_entry_extract(&one, &path1, &mode1);
+		else
+			/* two has more entries */
+			cmp = 1;
 		if (two.size)
 			elem2 = tree_entry_extract(&two, &path2, &mode2);
-
-		if (!one.size) {
-			/* two has more entries */
-			score += score_missing(mode2, path2);
-			update_tree_entry(&two);
-			continue;
-		}
-		if (!two.size) {
+		else
 			/* two lacks this entry */
-			score += score_missing(mode1, path1);
-			update_tree_entry(&one);
-			continue;
-		}
-		cmp = base_name_compare(path1, strlen(path1), mode1,
-					path2, strlen(path2), mode2);
+			cmp = -1;
+
+		if (!cmp)
+			cmp = base_name_compare(path1, strlen(path1), mode1,
+						path2, strlen(path2), mode2);
 		if (cmp < 0) {
 			/* path1 does not appear in two */
 			score += score_missing(mode1, path1);
-- 
1.8.2

Re: [PATCH 8/4] match-trees: drop "x = x" initializations

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Hmm, let's see if we can help the compiler follow the code without
> making it harder for people to understand.  The patch looks a bit
> jumbled, but the resulting code is OK in my biased opinion.

I actually think the result is much better than a mere "OK"; the
duplicated "at this point we know path1 (or path2) is missing from
the other side" has been bothering me and I was about to suggest a
similar rewrite before I read your message ;-)

However, the same compiler still thinks {elem,path,mode}1 can be
used uninitialized (but not {elem,path,mode}2).  The craziness I
reported in the previous message is also the same.  With this patch
on top to swap the side we inspect first, the compiler thinks
{elem,path,mode}2 can be used uninitialized but not the other three
variables X-<.

So I like your change for readability, but for GCC 4.4.5 we still
need the unnecessary initialization.

 match-trees.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/match-trees.c b/match-trees.c
index c0c66bb..9ea2c80 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -77,16 +77,16 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)
 		unsigned mode1, mode2;
 		int cmp = 0;
 
-		if (one.size)
-			elem1 = tree_entry_extract(&one, &path1, &mode1);
-		else
-			/* two has more entries */
-			cmp = 1;
 		if (two.size)
 			elem2 = tree_entry_extract(&two, &path2, &mode2);
 		else
 			/* two lacks this entry */
 			cmp = -1;
+		if (one.size)
+			elem1 = tree_entry_extract(&one, &path1, &mode1);
+		else
+			/* two has more entries */
+			cmp = 1;
 
 		if (!cmp)
 			cmp = base_name_compare(path1, strlen(path1), mode1,

Re: [PATCH 8/4] match-trees: drop "x = x" initializations

[Jeff King]

On Sat, Mar 23, 2013 at 09:55:53PM -0700, Junio C Hamano wrote:

> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
> > Hmm, let's see if we can help the compiler follow the code without
> > making it harder for people to understand.  The patch looks a bit
> > jumbled, but the resulting code is OK in my biased opinion.
> 
> I actually think the result is much better than a mere "OK"; the
> duplicated "at this point we know path1 (or path2) is missing from
> the other side" has been bothering me and I was about to suggest a
> similar rewrite before I read your message ;-)
> 
> However, the same compiler still thinks {elem,path,mode}1 can be
> used uninitialized (but not {elem,path,mode}2).  The craziness I
> reported in the previous message is also the same.  With this patch
> on top to swap the side we inspect first, the compiler thinks
> {elem,path,mode}2 can be used uninitialized but not the other three
> variables X-<.

Yeah, I'd agree that the result is more readable, but it does not
address the original problem. Junio, do you want to drop my patch,
squash the initialization of mode into René's version, and just add a
note to the commit message that we still have to deal with the gcc
warning?

-Peff

Re: [PATCH 8/4] match-trees: drop "x = x" initializations

[René Scharfe]

Am 24.03.2013 05:55, schrieb Junio C Hamano:
> However, the same compiler still thinks {elem,path,mode}1 can be
> used uninitialized (but not {elem,path,mode}2).  The craziness I
> reported in the previous message is also the same.  With this patch
> on top to swap the side we inspect first, the compiler thinks
> {elem,path,mode}2 can be used uninitialized but not the other three
> variables X-<.
> 
> So I like your change for readability, but for GCC 4.4.5 we still
> need the unnecessary initialization.

Hrm, perhaps we can make it even simpler for the compiler.

-- >8 --
Subject: match-trees: simplify score_trees() using tree_entry()

Convert the loop in score_trees() to tree_entry().  The code becomes
shorter and simpler because the calls to update_tree_entry() are not
needed any more.

Another benefit is that we need less variables to track the current
tree entries; as a side-effect of that the compiler has an easier
job figuring out the control flow and thus can avoid false warnings
about uninitialized variables.

Using struct name_entry also allows the use of tree_entry_len() for
finding the path length instead of strlen(), which may be slightly
more efficient.

Also unify the handling of missing entries in one of the two trees
(i.e. added or removed files): Just set cmp appropriately first, no
matter if we ran off the end of a tree or if we actually have two
entries to compare, and check its value a bit later without
duplicating the handler code.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
I'm a bit uneasy about this one because we lack proper tests for
this code and I don't know how to write ones off the bat.

 match-trees.c | 68 ++++++++++++++++++++++++-----------------------------------
 1 file changed, 28 insertions(+), 40 deletions(-)

diff --git a/match-trees.c b/match-trees.c
index 26f7ed1..2bb734d 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -47,6 +47,13 @@ static int score_matches(unsigned mode1, unsigned mode2, const char *path)
 	return score;
 }
 
+static int base_name_entries_compare(const struct name_entry *a,
+				     const struct name_entry *b)
+{
+	return base_name_compare(a->path, tree_entry_len(a), a->mode,
+				 b->path, tree_entry_len(b), b->mode);
+}
+
 /*
  * Inspect two trees, and give a score that tells how similar they are.
  */
@@ -71,54 +78,35 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)
 	if (type != OBJ_TREE)
 		die("%s is not a tree", sha1_to_hex(hash2));
 	init_tree_desc(&two, two_buf, size);
-	while (one.size | two.size) {
-		const unsigned char *elem1 = elem1;
-		const unsigned char *elem2 = elem2;
-		const char *path1 = path1;
-		const char *path2 = path2;
-		unsigned mode1 = mode1;
-		unsigned mode2 = mode2;
+	for (;;) {
+		struct name_entry e1, e2;
+		int got_entry_from_one = tree_entry(&one, &e1);
+		int got_entry_from_two = tree_entry(&two, &e2);
 		int cmp;
 
-		if (one.size)
-			elem1 = tree_entry_extract(&one, &path1, &mode1);
-		if (two.size)
-			elem2 = tree_entry_extract(&two, &path2, &mode2);
-
-		if (!one.size) {
-			/* two has more entries */
-			score += score_missing(mode2, path2);
-			update_tree_entry(&two);
-			continue;
-		}
-		if (!two.size) {
+		if (got_entry_from_one && got_entry_from_two)
+			cmp = base_name_entries_compare(&e1, &e2);
+		else if (got_entry_from_one)
 			/* two lacks this entry */
-			score += score_missing(mode1, path1);
-			update_tree_entry(&one);
-			continue;
-		}
-		cmp = base_name_compare(path1, strlen(path1), mode1,
-					path2, strlen(path2), mode2);
-		if (cmp < 0) {
+			cmp = -1;
+		else if (got_entry_from_two)
+			/* two has more entries */
+			cmp = 1;
+		else
+			break;
+
+		if (cmp < 0)
 			/* path1 does not appear in two */
-			score += score_missing(mode1, path1);
-			update_tree_entry(&one);
-			continue;
-		}
-		else if (cmp > 0) {
+			score += score_missing(e1.mode, e1.path);
+		else if (cmp > 0)
 			/* path2 does not appear in one */
-			score += score_missing(mode2, path2);
-			update_tree_entry(&two);
-			continue;
-		}
-		else if (hashcmp(elem1, elem2))
+			score += score_missing(e2.mode, e2.path);
+		else if (hashcmp(e1.sha1, e2.sha1))
 			/* they are different */
-			score += score_differs(mode1, mode2, path1);
+			score += score_differs(e1.mode, e2.mode, e1.path);
 		else
 			/* same subtree or blob */
-			score += score_matches(mode1, mode2, path1);
-		update_tree_entry(&one);
-		update_tree_entry(&two);
+			score += score_matches(e1.mode, e2.mode, e1.path);
 	}
 	free(one_buf);
 	free(two_buf);
-- 
1.8.2

Re: [PATCH 8/4] match-trees: drop "x = x" initializations

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Am 24.03.2013 05:55, schrieb Junio C Hamano:
>> So I like your change for readability, but for GCC 4.4.5 we still
>> need the unnecessary initialization.
>
> Hrm, perhaps we can make it even simpler for the compiler.

And the result is even simpler for human readers, I'd have to say.

> I'm a bit uneasy about this one because we lack proper tests for
> this code and I don't know how to write ones off the bat.

This looks pretty much a straight-forward equivalent rewrite from
your earlier one, which was also an obvious equivalent to the
original, at least to me.  The first four lines in the original were
made into two tree_entry() calls (what a useful helper we haven't
been using!) and that allows us to lose explicit update_tree_entry()
calls.



>  match-trees.c | 68 ++++++++++++++++++++++++-----------------------------------
>  1 file changed, 28 insertions(+), 40 deletions(-)
>
> diff --git a/match-trees.c b/match-trees.c
> index 26f7ed1..2bb734d 100644
> --- a/match-trees.c
> +++ b/match-trees.c
> @@ -47,6 +47,13 @@ static int score_matches(unsigned mode1, unsigned mode2, const char *path)
>  	return score;
>  }
>  
> +static int base_name_entries_compare(const struct name_entry *a,
> +				     const struct name_entry *b)
> +{
> +	return base_name_compare(a->path, tree_entry_len(a), a->mode,
> +				 b->path, tree_entry_len(b), b->mode);
> +}
> +
>  /*
>   * Inspect two trees, and give a score that tells how similar they are.
>   */
> @@ -71,54 +78,35 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)
>  	if (type != OBJ_TREE)
>  		die("%s is not a tree", sha1_to_hex(hash2));
>  	init_tree_desc(&two, two_buf, size);
> -	while (one.size | two.size) {
> -		const unsigned char *elem1 = elem1;
> -		const unsigned char *elem2 = elem2;
> -		const char *path1 = path1;
> -		const char *path2 = path2;
> -		unsigned mode1 = mode1;
> -		unsigned mode2 = mode2;
> +	for (;;) {
> +		struct name_entry e1, e2;
> +		int got_entry_from_one = tree_entry(&one, &e1);
> +		int got_entry_from_two = tree_entry(&two, &e2);
>  		int cmp;
>  
> -		if (one.size)
> -			elem1 = tree_entry_extract(&one, &path1, &mode1);
> -		if (two.size)
> -			elem2 = tree_entry_extract(&two, &path2, &mode2);
> -
> -		if (!one.size) {
> -			/* two has more entries */
> -			score += score_missing(mode2, path2);
> -			update_tree_entry(&two);
> -			continue;
> -		}
> -		if (!two.size) {
> +		if (got_entry_from_one && got_entry_from_two)
> +			cmp = base_name_entries_compare(&e1, &e2);
> +		else if (got_entry_from_one)
>  			/* two lacks this entry */
> -			score += score_missing(mode1, path1);
> -			update_tree_entry(&one);
> -			continue;
> -		}
> -		cmp = base_name_compare(path1, strlen(path1), mode1,
> -					path2, strlen(path2), mode2);
> -		if (cmp < 0) {
> +			cmp = -1;
> +		else if (got_entry_from_two)
> +			/* two has more entries */
> +			cmp = 1;
> +		else
> +			break;
> +
> +		if (cmp < 0)
>  			/* path1 does not appear in two */
> -			score += score_missing(mode1, path1);
> -			update_tree_entry(&one);
> -			continue;
> -		}
> -		else if (cmp > 0) {
> +			score += score_missing(e1.mode, e1.path);
> +		else if (cmp > 0)
>  			/* path2 does not appear in one */
> -			score += score_missing(mode2, path2);
> -			update_tree_entry(&two);
> -			continue;
> -		}
> -		else if (hashcmp(elem1, elem2))
> +			score += score_missing(e2.mode, e2.path);
> +		else if (hashcmp(e1.sha1, e2.sha1))
>  			/* they are different */
> -			score += score_differs(mode1, mode2, path1);
> +			score += score_differs(e1.mode, e2.mode, e1.path);
>  		else
>  			/* same subtree or blob */
> -			score += score_matches(mode1, mode2, path1);
> -		update_tree_entry(&one);
> -		update_tree_entry(&two);
> +			score += score_matches(e1.mode, e2.mode, e1.path);
>  	}
>  	free(one_buf);
>  	free(two_buf);

Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings

[Joachim Schmitz]

Johannes Sixt wrote:
> Am 3/21/2013 12:03, schrieb Jeff King:
>> I was fooling around with clang and noticed that it complains about
>> the "int x = x" construct under -Wall. That is IMHO a deficiency in
>> clang, since the idiom has a well-defined use in silencing
>> -Wuninitialized warnings.
>
> IMO, that's a myth. The construct invokes undefined behavior at least
> since C99, and the compilers are right to complain about it.

And I complained about this a couple months ago, as the compiler on 
HP-NonStop stumbles across this too (by emitting a warning)

Bye, Jojo 

Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings

[Joachim Schmitz]

Joachim Schmitz wrote:
> Johannes Sixt wrote:
>> Am 3/21/2013 12:03, schrieb Jeff King:
>>> I was fooling around with clang and noticed that it complains about
>>> the "int x = x" construct under -Wall. That is IMHO a deficiency in
>>> clang, since the idiom has a well-defined use in silencing
>>> -Wuninitialized warnings.
>> 
>> IMO, that's a myth. The construct invokes undefined behavior at least
>> since C99, and the compilers are right to complain about it.
> 
> And I complained about this a couple months ago, as the compiler on

Actually on August 20th, 2012...

> HP-NonStop stumbles across this too (by emitting a warning)


Bye, Jojo