[PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning

[PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning

[Ramsay Jones]

After commit cbfd5e1c ("drop some obsolete "x = x" compiler warning
hacks", 21-03-2013) removed a gcc specific hack, older versions of
gcc now issue an "'contents' might be used uninitialized" warning.
In order to suppress the warning, we simply initialize the variable
to NULL in it's declaration.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

An alternative solution may look like this (note: *untested*):

    diff --git a/builtin/cat-file.c b/builtin/cat-file.c
    index ad29000..e50b20f 100644
    --- a/builtin/cat-file.c
    +++ b/builtin/cat-file.c
    @@ -193,7 +193,6 @@ 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;
     
     	if (!obj_name)
     	   return 1;
    @@ -204,16 +203,11 @@ static int batch_one_object(const char *obj_name, int print_contents)
     		return 0;
     	}
     
    -	if (print_contents == BATCH)
    -		contents = read_sha1_file(sha1, &type, &size);
    -	else
    -		type = sha1_object_info(sha1, &size);
    +	type = sha1_object_info(sha1, &size);
     
     	if (type <= 0) {
     		printf("%s missing\n", obj_name);
     		fflush(stdout);
    -		if (print_contents == BATCH)
    -			free(contents);
     		return 0;
     	}
     
    @@ -221,6 +215,7 @@ static int batch_one_object(const char *obj_name, int print_contents)
     	fflush(stdout);
     
     	if (print_contents == BATCH) {
    +		void *contents = read_sha1_file(sha1, &type, &size);
     		write_or_die(1, contents, size);
     		printf("\n");
     		fflush(stdout);
    -- 

However, this would add an additional call to sha1_object_info() to
the "--batch" code path, with potential performance consequences
(again untested). Also, if you are paranoid, I guess you should
check that the (type,size) returned by sha1_object_info() was the
same as that returned by read_sha1_file(). ;-)

ATB,
Ramsay Jones

 builtin/cat-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index ad29000..40f87b4 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;
+	void *contents = NULL;
 
 	if (!obj_name)
 	   return 1;
-- 
1.8.2

Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning

[Jeff King]

On Tue, Mar 26, 2013 at 07:20:11PM +0000, Ramsay Jones wrote:

> After commit cbfd5e1c ("drop some obsolete "x = x" compiler warning
> hacks", 21-03-2013) removed a gcc specific hack, older versions of
> gcc now issue an "'contents' might be used uninitialized" warning.
> In order to suppress the warning, we simply initialize the variable
> to NULL in it's declaration.

I'm OK with this, if it's the direction we want to go. But I thought the
discussion kind of ended as "we do not care about these warnings on
ancient versions of gcc; those people should use -Wno-error=uninitialized".

What version of gcc are you using? If it is the most recent thing
reasonably available on msysgit, then I am more sympathetic. But if it's
just an antique version of gcc, I am less so.

> An alternative solution may look like this (note: *untested*):
> [...]
> However, this would add an additional call to sha1_object_info()

Yeah, I don't think that is worth it.

> the "--batch" code path, with potential performance consequences
> (again untested). Also, if you are paranoid, I guess you should
> check that the (type,size) returned by sha1_object_info() was the
> same as that returned by read_sha1_file(). ;-)

I note that we do not actually check that contents != NULL after calling
read_sha1_file, either (nor that sha1_object_info does not return an
error). I suspect cat-file could segfault under the right conditions.

-Peff

Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning

[Jeff King]

On Tue, Mar 26, 2013 at 03:35:39PM -0400, Jeff King wrote:

> I note that we do not actually check that contents != NULL after calling
> read_sha1_file, either (nor that sha1_object_info does not return an
> error). I suspect cat-file could segfault under the right conditions.

Oh nevermind, we do. We just do it by checking that "type" was filled in
correctly, which can combine the check for both read_sha1_file and
sha1_object_info. Sorry for the noise.

-Peff

Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning

[Ramsay Jones]

Jeff King wrote:
> On Tue, Mar 26, 2013 at 07:20:11PM +0000, Ramsay Jones wrote:
> 
>> After commit cbfd5e1c ("drop some obsolete "x = x" compiler warning
>> hacks", 21-03-2013) removed a gcc specific hack, older versions of
>> gcc now issue an "'contents' might be used uninitialized" warning.
>> In order to suppress the warning, we simply initialize the variable
>> to NULL in it's declaration.
> 
> I'm OK with this, if it's the direction we want to go. But I thought the
> discussion kind of ended as "we do not care about these warnings on
> ancient versions of gcc; those people should use -Wno-error=uninitialized".

Hmm, I don't recall any agreement or conclusions being reached.
I guess I missed that!

> What version of gcc are you using? If it is the most recent thing
> reasonably available on msysgit, then I am more sympathetic. But if it's
> just an antique version of gcc, I am less so.

(see previous email for compiler versions).

I suppose it depends on what you consider antique. [I recently
downloaded the "first C compiler" from github. Yes, that is an
antique compiler! ;-)] I would call some of the compilers I use
"a bit mature." :-P

Hmm, so are you saying that this patch is not acceptable because
I used a compiler that is no longer supported?

ATB,
Ramsay Jones

Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning

[Jeff King]

On Thu, Mar 28, 2013 at 06:48:43PM +0000, Ramsay Jones wrote:

> > I'm OK with this, if it's the direction we want to go. But I thought the
> > discussion kind of ended as "we do not care about these warnings on
> > ancient versions of gcc; those people should use -Wno-error=uninitialized".
> 
> Hmm, I don't recall any agreement or conclusions being reached.
> I guess I missed that!

I think Jonathan said that and nobody disagreed, and I took it as a
conclusion.

> Hmm, so are you saying that this patch is not acceptable because
> I used a compiler that is no longer supported?

No, I just think we should come to a decision on how unreadable to make
the code in order to suppress incorrect warnings on old compilers. I can
see the point in either of the following arguments:

  1. These compilers are old, and we do not need to cater to them in the
     code because people can just _not_ set -Werror=uninitialized (or
     its equivalent). It is still worth catering to bugs in modern
     compilers that most devs use, because being able to set -Werror is
     helpful.

  2. The code is not made significantly less readable, especially if you
     put in a comment, so why not help these compilers.

When we can make the code more readable _and_ help the compiler, I think
it is a no-brainer. I am on the fence otherwise and don't care that
much. I just think we should apply the rule consistently.

-Peff

Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning

[Jonathan Nieder]

Jeff King wrote:

> When we can make the code more readable _and_ help the compiler, I think
> it is a no-brainer.

Yep. :)