threads / patch / 36421

patchconfig.c: fix a compiler warning

Subject: [PATCH] config.c: fix a compiler warning

## tl;dr

5 messages between Apr 16, 2014 and Apr 16, 2014. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

Stepan Kasal· Apr 16, 2014, 14:13 UTC · lore

Date: Thu, 10 Apr 2014 16:37:15 +0200

This change fixes a gcc warning when building msysGit.
---
 config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Show changes to config.c +2 −2

diff --git a/config.c b/config.c
index 314d8ee..0b7e4f8 100644
--- a/config.c
+++ b/config.c
@@ -572,7 +572,7 @@ static void die_bad_number(const char *name, const char *value)
 
 int git_config_int(const char *name, const char *value)
 {
-	int ret;
+	int ret = 0;
 	if (!git_parse_int(value, &ret))
 		die_bad_number(name, value);
 	return ret;
@@ -580,7 +580,7 @@ int git_config_int(const char *name, const char *value)
 
 int64_t git_config_int64(const char *name, const char *value)
 {
-	int64_t ret;
+	int64_t ret = 0;
 	if (!git_parse_int64(value, &ret))
 		die_bad_number(name, value);
 	return ret;

-- 
1.9.2.msysgit.0.154.g978f18d

Jeff King· Apr 16, 2014, 15:29 UTC · re: Stepan Kasal · lore

Re: [PATCH] config.c: fix a compiler warning

On Wed, Apr 16, 2014 at 04:13:53PM +0200, Stepan Kasal wrote:

> Date: Thu, 10 Apr 2014 16:37:15 +0200
> 
> This change fixes a gcc warning when building msysGit.

What warning? I'm assuming -Wuninitialized?

Show 13 quoted lines

> diff --git a/config.c b/config.c
> index 314d8ee..0b7e4f8 100644
> --- a/config.c
> +++ b/config.c
> @@ -572,7 +572,7 @@ static void die_bad_number(const char *name, const char *value)
>  
>  int git_config_int(const char *name, const char *value)
>  {
> -	int ret;
> +	int ret = 0;
>  	if (!git_parse_int(value, &ret))
>  		die_bad_number(name, value);
>  	return ret;

Hmph. Generally gcc should assume that a variable is initialized after a pointer to it is passed into a function. Unless it inlines that function and can see that it isn't. But if we do inline git_parse_int, we see that it "ret" is always initialized if it returns non-zero.

If it also inlines die_bad_number, it would see that we end in die(), which is marked NORETURN. But if it does not, it will not realize that we do not get to "return ret" in that case.

So perhaps a better solution is:

Show changes to config.c +1 −1

diff --git a/config.c b/config.c
index 6821cef..a30cb5c 100644
--- a/config.c
+++ b/config.c
@@ -557,6 +557,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)
 	return 1;
 }
 
+NORETURN
 static void die_bad_number(const char *name, const char *value)
 {
 	const char *reason = errno == ERANGE ?

Does that also silence the warning?

-Peff

Torsten Bögershausen· Apr 16, 2014, 16:38 UTC · re: Jeff King · lore

Re: [PATCH] config.c: fix a compiler warning

On 2014-04-16 17.29, Jeff King wrote:

Show 5 quoted lines

> On Wed, Apr 16, 2014 at 04:13:53PM +0200, Stepan Kasal wrote:
> 
>> Date: Thu, 10 Apr 2014 16:37:15 +0200
>>
>> This change fixes a gcc warning when building msysGit.

[]

Show 8 quoted lines

> +NORETURN
>  static void die_bad_number(const char *name, const char *value)
>  {
>  	const char *reason = errno == ERANGE ?
> 
> Does that also silence the warning?
> 
> -Peff

This works under gcc 4.2.1 Mac OS: the warning is away.

Jeff King· Apr 16, 2014, 16:51 UTC · re: Torsten Bögershausen · lore

[PATCH] config.c: mark die_bad_number as NORETURN

On Wed, Apr 16, 2014 at 06:38:19PM +0200, Torsten Bögershausen wrote:

> > Does that also silence the warning?
> > 
> This works under gcc 4.2.1 Mac OS: the warning is away.

Thanks. I couldn't test myself, as I could not get gcc to generate the warning in the first place, but I do not have anything as old as 4.2 on hand.

Here it is with a commit message.

-- >8 --
Subject: config.c: mark die_bad_number as NORETURN

This can help avoid -Wuninitialized false positives in git_config_int and git_config_ulong, as the compiler now knows that we do not return "ret" if we hit the error codepath.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c | 1 +
 1 file changed, 1 insertion(+)

Show changes to config.c +1 −0

diff --git a/config.c b/config.c
index 6821cef..a30cb5c 100644
--- a/config.c
+++ b/config.c
@@ -557,6 +557,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)
 	return 1;
 }
 
+NORETURN
 static void die_bad_number(const char *name, const char *value)
 {
 	const char *reason = errno == ERANGE ?

-- 
1.9.1.656.ge8a0637

Junio C Hamano· Apr 16, 2014, 17:21 UTC · re: Jeff King · lore

Re: [PATCH] config.c: mark die_bad_number as NORETURN

Jeff King <peff@peff.net> writes:

Show 19 quoted lines

> On Wed, Apr 16, 2014 at 06:38:19PM +0200, Torsten Bögershausen wrote:
>
>> > Does that also silence the warning?
>> > 
>> This works under gcc 4.2.1 Mac OS: the warning is away.
>
> Thanks. I couldn't test myself, as I could not get gcc to generate the
> warning in the first place, but I do not have anything as old as 4.2 on
> hand.
>
> Here it is with a commit message.
>
> -- >8 --
> Subject: config.c: mark die_bad_number as NORETURN
>
> This can help avoid -Wuninitialized false positives in
> git_config_int and git_config_ulong, as the compiler now
> knows that we do not return "ret" if we hit the error
> codepath.

Thanks for clearly solving the issue and describing the solution. Will queue.

Show 17 quoted lines

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  config.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/config.c b/config.c
> index 6821cef..a30cb5c 100644
> --- a/config.c
> +++ b/config.c
> @@ -557,6 +557,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)
>  	return 1;
>  }
>  
> +NORETURN
>  static void die_bad_number(const char *name, const char *value)
>  {
>  	const char *reason = errno == ERANGE ?