threads / patch / 20348

patchFix compiler warning by properly initialize failed_errno

Subject: [PATCH] Fix compiler warning by properly initialize failed_errno

## tl;dr

6 messages between Aug 2, 2009 and Aug 4, 2009. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

David Soria Parra· Aug 2, 2009, 19:34 UTC · lore
From: David Soria Parra <dsp@php.net>
Initilize failed_error in start_command to avoid compiler warnings
Signed-off-by: David Soria Parra <dsp@php.net>
---
 run-command.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to run-command.c +1 −1
diff --git a/run-command.c b/run-command.c
index dc09433..510349b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -19,7 +19,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;
+	int failed_errno = 0;
 
 	/*
 	 * In case of errors we must keep the promise to close FDs

-- 
1.6.4.212.g4719.dirty
Junio C Hamano· Aug 4, 2009, 06:07 UTC · re: David Soria Parra · lore

Re: [PATCH] Fix compiler warning by properly initialize failed_errno

David Soria Parra <sn_@gmx.net> writes:
Show 22 quoted lines
> From: David Soria Parra <dsp@php.net>
>
> Initilize failed_error in start_command to avoid compiler warnings
>
> Signed-off-by: David Soria Parra <dsp@php.net>
> ---
>  run-command.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index dc09433..510349b 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -19,7 +19,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;
> +	int failed_errno = 0;
>  
>  	/*
>  	 * In case of errors we must keep the promise to close FDs

We would want to be able to distinguish between a workaround for a compiler that is not clever/careful enough, and a necessary initialization. In this particular case, it is the former, and we should say

	int failed_errno = failed_errno;
instead.

The potentially uninitialized use your compiler is worried about is inside if (cmd->pid < 0) after #ifdef/#else/#endif.

 (1) if not on MINGW32, we would have already assigned to failed_errno
     after fork() returns negative value to cmd->pid;
 (2) if on MINGW32, we would have assigned to failed_errno unconditionally
     after calling mingw_spawnvpe().
so its worry is unfounded.
sn_· Aug 4, 2009, 09:27 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix compiler warning by properly initialize failed_errno

Show 10 quoted lines
> The potentially uninitialized use your compiler is worried about is inside
> if (cmd->pid < 0) after #ifdef/#else/#endif.
> 
>  (1) if not on MINGW32, we would have already assigned to failed_errno
>      after fork() returns negative value to cmd->pid;
> 
>  (2) if on MINGW32, we would have assigned to failed_errno unconditionally
>      after calling mingw_spawnvpe().
> 
> so its worry is unfounded.
The worry is definatly unfounded, but I think it's still worth to apply the attached patch to get rid of the warning using the i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5490) compiler. I sended a corrected version of the patch to the ml.
-- 
Jetzt kostenlos herunterladen: Internet Explorer 8 und Mozilla Firefox 3 -
sicherer, schneller und einfacher! http://portal.gmx.net/de/go/atbrowser
Junio C Hamano· Aug 4, 2009, 22:22 UTC · re: sn_ · lore

Re: [PATCH] Fix compiler warning by properly initialize failed_errno

"sn_" <sn_@gmx.net> writes:
Show 15 quoted lines
>> The potentially uninitialized use your compiler is worried about is inside
>> if (cmd->pid < 0) after #ifdef/#else/#endif.
>> 
>>  (1) if not on MINGW32, we would have already assigned to failed_errno
>>      after fork() returns negative value to cmd->pid;
>> 
>>  (2) if on MINGW32, we would have assigned to failed_errno unconditionally
>>      after calling mingw_spawnvpe().
>> 
>> so its worry is unfounded.
>
> The worry is definatly unfounded, but I think it's still worth to apply
> the attached patch to get rid of the warning using the
> i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5490)
> compiler. I sended a corrected version of the patch to the ml.

Oh, there was no need for you to say "but..." and everything that followed. I said "we should say ... instead" in my review comments, didn't I? 

We are obviously in agreement ;-)
Johannes Sixt· Aug 4, 2009, 18:51 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix compiler warning by properly initialize failed_errno

Junio C Hamano schrieb:
Show 33 quoted lines
> David Soria Parra <sn_@gmx.net> writes:
> 
>> From: David Soria Parra <dsp@php.net>
>>
>> Initilize failed_error in start_command to avoid compiler warnings
>>
>> Signed-off-by: David Soria Parra <dsp@php.net>
>> ---
>>  run-command.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index dc09433..510349b 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -19,7 +19,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;
>> +	int failed_errno = 0;
>>  
>>  	/*
>>  	 * In case of errors we must keep the promise to close FDs
> 
> We would want to be able to distinguish between a workaround for a
> compiler that is not clever/careful enough, and a necessary
> initialization.  In this particular case, it is the former, and we should
> say
> 
> 	int failed_errno = failed_errno;
> 
> instead.

Frankly, I prefer the initialization with 0; this is not a performance critical place and micro-optimization is not appropriate here.

(If this were C++ then I *know* that int x = x; is undefined behavior, strictly speaking; I don't know whether it is the same with C.)

Nevertheless, for both versions:
Acked-by: Johannes Sixt <j6t@kdbg.org>
-- Hannes
Junio C Hamano· Aug 4, 2009, 19:09 UTC · re: Johannes Sixt · lore

Re: [PATCH] Fix compiler warning by properly initialize failed_errno

Johannes Sixt <j6t@kdbg.org> writes:
Show 13 quoted lines
> Junio C Hamano schrieb:
>
>> We would want to be able to distinguish between a workaround for a
>> compiler that is not clever/careful enough, and a necessary
>> initialization.  In this particular case, it is the former, and we should
>> say
>>
>> 	int failed_errno = failed_errno;
>>
>> instead.
>
> Frankly, I prefer the initialization with 0; this is not a performance
> critical place and micro-optimization is not appropriate here.

It is not about optimization at all. This is about documenting the fact that we have audited and know that the use of this variable in the code that follows is Ok. Initializing to 0 gives a false impression that the code may rely on that value, but in this case nobody will ever read that zero before overwriting it with an assignment.

The compiler may optimize this out, but that is an insignificant (I agree this is not a performance critical codepath) side effect.

← back to recent threads