threads / patch / 21681

patchsubmodule.c: Squelch a "use before assignment" warning

Subject: [PATCH] submodule.c: Squelch a "use before assignment" warning

## tl;dr

6 messages between Nov 20, 2009 and Nov 22, 2009. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

David Aguilar· Nov 20, 2009, 01:33 UTC · lore

i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5493) compiler (and probably others) mistakenly thinks variable 'right' is used before assigned. Work it around by giving it a fake initialization.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 submodule.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to submodule.c +1 −1
diff --git a/submodule.c b/submodule.c
index 461faf0..0145a62 100644
--- a/submodule.c
+++ b/submodule.c
@@ -38,7 +38,7 @@ void show_submodule_summary(FILE *f, const char *path,
 		const char *del, const char *add, const char *reset)
 {
 	struct rev_info rev;
-	struct commit *commit, *left = left, *right;
+	struct commit *commit, *left = left, *right = NULL;
 	struct commit_list *merge_bases, *list;
 	const char *message = NULL;
 	struct strbuf sb = STRBUF_INIT;

-- 
1.6.5.3.171.ge36e
Junio C Hamano· Nov 20, 2009, 08:05 UTC · re: David Aguilar · lore

Re: [PATCH] submodule.c: Squelch a "use before assignment" warning

David Aguilar <davvid@gmail.com> writes:
> i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5493) compiler
> (and probably others) mistakenly thinks variable 'right' is used
> before assigned.  Work it around by giving it a fake initialization.

We see the same "fake initialization" of 'left' on the same line. By initializing it to NULL, you are hinting that initializing 'right' to NULL actually means something.

Show 10 quoted lines
> diff --git a/submodule.c b/submodule.c
> index 461faf0..0145a62 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -38,7 +38,7 @@ void show_submodule_summary(FILE *f, const char *path,
>  		const char *del, const char *add, const char *reset)
>  {
>  	struct rev_info rev;
> -	struct commit *commit, *left = left, *right;
> +	struct commit *commit, *left = left, *right = NULL;
David Aguilar· Nov 20, 2009, 11:33 UTC · re: Junio C Hamano · lore

i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5493) compiler (and probably others) mistakenly thinks variable 'right' is used before assigned. Work around it by giving it a fake initialization.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 submodule.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to submodule.c +1 −1
diff --git a/submodule.c b/submodule.c
index 461faf0..86aad65 100644
--- a/submodule.c
+++ b/submodule.c
@@ -38,7 +38,7 @@ void show_submodule_summary(FILE *f, const char *path,
 		const char *del, const char *add, const char *reset)
 {
 	struct rev_info rev;
-	struct commit *commit, *left = left, *right;
+	struct commit *commit, *left = left, *right = right;
 	struct commit_list *merge_bases, *list;
 	const char *message = NULL;
 	struct strbuf sb = STRBUF_INIT;

-- 
1.6.5.3.171.ge36e
Christoph Bartoschek· Nov 21, 2009, 18:46 UTC · re: Junio C Hamano · lore

Re: [PATCH] submodule.c: Squelch a "use before assignment" warning

Junio C Hamano wrote:
Show 9 quoted lines
> David Aguilar <davvid@gmail.com> writes:
> 
>> i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5493) compiler
>> (and probably others) mistakenly thinks variable 'right' is used
>> before assigned.  Work it around by giving it a fake initialization.
> 
> We see the same "fake initialization" of 'left' on the same line.  By
> initializing it to NULL, you are hinting that initializing 'right' to
> NULL actually means something.

Why is the compiler not complaining about the fake initalization? For initialization a value is used that is not initialized.

At least a static analyzer complains:

"submodule.c", line 41: The variable `left' is used before its initialization 

Christoph
Junio C Hamano· Nov 22, 2009, 02:59 UTC · re: Christoph Bartoschek · lore

Re: [PATCH] submodule.c: Squelch a "use before assignment" warning

Christoph Bartoschek <bartoschek@gmx.de> writes:
> Why is the compiler not complaining about the fake initalization? For 
> initialization a value is used that is not initialized.

That is a fairly well established idiom to tell gcc "you may mistakenly think it isn't, but this is used".

Junio C Hamano· Nov 22, 2009, 03:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] submodule.c: Squelch a "use before assignment" warning

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> Christoph Bartoschek <bartoschek@gmx.de> writes:
>
>> Why is the compiler not complaining about the fake initalization? For 
>> initialization a value is used that is not initialized.
>
> That is a fairly well established idiom to tell gcc "you may mistakenly
> think it isn't, but this is used".

Sorry, but I was called away from the keyboard in mid sentence. It should have read "...this is used after getting assigned, so do not worry".

But I need to clarify a few things about this issue.

As the maintainer, I do not like a fake initialisation myself very much. It is a declaration that "left" is always assigned before getting used by the person who wrote _that particular version_, but later updates to the code might introduce a codepath to incorrectly use "left" before getting assinged to, and the fake initialisation will prevent the compiler from catching it.

When the variable in question is a pointer, assigning NULL instead of "left = left" will at least give us a predictable and more reproducible breakage when later updates break the code. I wouldn't have minded if David's patch were to update both the existing fake initialisation and the new one to assign NULL. At least dereferencing such a pointer will give us a segfault reliably. But unfortunately there is no such "magic" values for variables of other types (e.g. int) to help us catch uninitialized use of variables at runtime.

By the way, if a static analyser is meant to be useful for real-world programs, as opposed to merely an academic exercise, it should know this convention; like it or not, it is used fairly widely. That is, it should check "left" is assigned before used in the rest of the function without this "gcc hack" initializer, and if the only questionable use of "left" is the RHS of this fake initialisation, should refrain from warning.

← back to recent threads