[PATCH] Fix compiler warning by properly initialize failed_errno

[PATCH] Fix compiler warning by properly initialize failed_errno

[David Soria Parra]

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
-- 
1.6.4.212.g4719.dirty

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

[Junio C Hamano]

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.

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.

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

[sn_]

> 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

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

[Johannes Sixt]

Junio C Hamano schrieb:
> 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

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> 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.

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

[Junio C Hamano]

"sn_" <sn_@gmx.net> writes:

>> 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 ;-)