Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning
Jeff King wrote: > When we can make the code more readable _and_ help the compiler, I think > it is a no-brainer. Yep. :) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning
On Thu, Mar 28, 2013 at 06:48:43PM +, 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning
Jeff King wrote:
> On Tue, Mar 26, 2013 at 07:20:11PM +, 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning
On Tue, Mar 26, 2013 at 07:20:11PM +, 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
