"Joachim Schmitz" [off-list ref] writes:
OK, so let's have a look at code, current git,
builtin/cat-file.c, line 196:
void *contents = contents;
This variable is set later in an if branch (if (print_contents == BATCH),
but not in the else branch. It is later used always under the same condition
as the one under which it is set.
Apparently is is malloc_d storage (there a "free(content);"), so there's no
harm al all in initializing it with NULL, even if it only appeases a stupid
compiler.
It actually is harmful. See below.
The next one, builtin/fast-export.c, line 486:
struct commit *commit = commit;
it is set in a switch statement, but not in every case, as far as I can see.
Hmm, maybe it is, and I just get lost in the code
And it is used directly after the switch, hopefully set to something
reasonable.
Why take the risk and not set it to NULL?
Ditto.
Next one, builtin/rev-list.c, line 390:
int reaches = reaches, all = all;
revs.commits = find_bisection(revs.commits, &reaches, &all,
bisect_find_all);
Seem pretty pointless to initialize them, provided find_bisection doesn't
read them. Does it?
That is why they are not initializations but marks to the compiler
to signal "you may be stupid enough to think they are used before
initialized or assigned, but that is not the case". Initializing
them would be pointless.
Next one, fast-import.c, line 2268:
struct object_entry *oe = oe;
os gets set in en if and an else branch, but not in then intermediate else
if branch!
Look again. If the recent code is too complex for you to
understand, go back to 10e8d68 (Correct compiler warnings in
fast-import., 2007-02-06) and read the function.
The control flow of the early part of that function dictates that
either oe is assigned *or* inline_data is set to 1. When inline_data
is false, oe is always set.
The compiler was too stupid to read that, and that is why the
(confusing) idiom to mark it for the stupid compiler was used.
There are a few reasons why I do not think this self-assignment
idiom or initializing the variable to an innocuous-looking random
value is a particularly good thing to do when you see compiler
warnings.
If the compiler suspects the variable might be unused, you should
always look at it and follow the flow yourself. Once you know it is
a false alarm, you can use the idiom to squelch the warning, and it
at the same serves as a note to others that you verified the flow
and made sure it is a false warning.
When the next person wants to touch the code, if the person knows
the use of the idiom, it only serves as a warning to be extra
careful not to introduce a new codepath that reads the variable
without setting, as the compiler no longer helps him. If the person
who touches the code is as clueless as the compiler and cannot
follow the codepath to see the variable is never used uninitialized,
the result will be a lot worse.
That is the reason why I do not think the idiom to squelch the
compiler is such a good thing. Careless people touch the code, so
"oe = oe" initialization carefully placed in the original version
does not necessarily stay as a useful documentation.
But if you use "oe = NULL" as a way to squelch the warning in the
first place, it is no better than "oe = oe". In a sense, it is even
worse, as it just looks like any other initialization and gives a
false impression that the remainder of the code is written in such a
way that it tolerates oe being NULL in any codepath, or there is
some assignment before that before the code reaches places where oe
cannot be NULL. That is different from what "oe = oe" initializaion
documents---in the codepath protected by "if (inline_data)", it
isn't just "oe can safely be NULL there"; instead it is "oe can
safely be *any* value there, because we don't use it". Of course,
if you explicitly initialized oe to NULL, even if you introduce a
codepath where oe cannot be NULL later, you won't get a warning from
the compiler, so it is no better than "oe = oe".
And that is the reason why I do not think initialization to an
innocuous-looking random value (e.g. NULL) is a good answer, either.
When both are not good, replacing "oe = oe" with "oe = NULL" didn't
make much sense, especially when the former _could_ be used better
by more careful people. And that is the resistance J6t remembers.
But when recent compilers started to warn "oe = oe" that itself is
undefined, the equation changed. The idiom ceased to be a way to
squelch the incorrect compiler warning (which was the primary point
of its use---the documentation value is secondary, as what we
document is "we are squelching a false alarm", but we no longer are
squelching anything).
See 4a5de8d (vcs-svn: avoid self-assignment in dummy initialization
of pre_off, 2012-06-01), and 58ebd98 (vcs-svn/svndiff.c: squelch
false "unused" warning from gcc, 2012-01-27) that it updated, for an
example of how we are migrating away from "oe = oe" idiom.
In any case, I already said initializing to 0 is OK in my previous
message, I think, so if you are seeing warning from the compiler on
uninitialized variables in your new code, you know you should
(1) first follow the codepath to make sure it is a false alarm (if
it isn't, fix the code);
(2) then see if you can simplify your code so that it is crystal
clear even to the stupid compiler that you are not using an
uninitialized variable; and
(3) if you can't, then initialize it to some known bad value,
avoiding the "oe = oe" idiom.
Is there anything more to discuss on this topic?
-----Original Message-----
From: Junio C Hamano [mailto:gitster@pobox.com]
Sent: Tuesday, August 21, 2012 4:06 AM
To: Joachim Schmitz
Cc: 'Johannes Sixt'; 'Jan Engelhardt'; git@vger.kernel.org
Subject: Re: git on HP NonStop
"Joachim Schmitz" [off-list ref] writes:
quoted
OK, so let's have a look at code, current git, builtin/cat-file.c,
line 196:
void *contents = contents;
This variable is set later in an if branch (if (print_contents ==
BATCH), but not in the else branch. It is later used always under the
same condition as the one under which it is set.
Apparently is is malloc_d storage (there a "free(content);"), so
there's no harm al all in initializing it with NULL, even if it only
appeases a stupid compiler.
It actually is harmful. See below.
Harmful to initialize with NULL or to use that undefined behavoir?
I checked what our compiler does here: after having warned about "vlues us
used before it is set: it actually dies seem to have initializes the value
to 0 resp. NULL.
So here there's no harm done in avoiding undefined behavior and set it to 0
resp NULL in the first place.
quoted
The next one, builtin/fast-export.c, line 486:
struct commit *commit = commit; it is set in a switch
statement, but not in every case, as far as I can see.
Hmm, maybe it is, and I just get lost in the code And it is used
directly after the switch, hopefully set to something reasonable.
Why take the risk and not set it to NULL?
Ditto.
quoted
Next one, builtin/rev-list.c, line 390:
int reaches = reaches, all = all;
revs.commits = find_bisection(revs.commits, &reaches,
&all,
quoted
bisect_find_all);
Seem pretty pointless to initialize them, provided find_bisection
doesn't read them. Does it?
That is why they are not initializations but marks to the compiler to
signal "you
may be stupid enough to think they are used before initialized or
assigned, but
that is not the case". Initializing them would be pointless.
quoted
Next one, fast-import.c, line 2268:
struct object_entry *oe = oe;
os gets set in en if and an else branch, but not in then intermediate
else if branch!
Look again. If the recent code is too complex for you to understand, go
back to
10e8d68 (Correct compiler warnings in fast-import., 2007-02-06) and read
the
function.
The control flow of the early part of that function dictates that either
oe is
assigned *or* inline_data is set to 1. When inline_data is false, oe is
always
set.
The compiler was too stupid to read that, and that is why the
(confusing) idiom to mark it for the stupid compiler was used.
There are a few reasons why I do not think this self-assignment idiom or
initializing the variable to an innocuous-looking random value is a
particularly
good thing to do when you see compiler warnings.
If the compiler suspects the variable might be unused, you should always
look
at it and follow the flow yourself. Once you know it is a false alarm,
you can
use the idiom to squelch the warning, and it at the same serves as a note
to
others that you verified the flow and made sure it is a false warning.
When the next person wants to touch the code, if the person knows the use
of
the idiom, it only serves as a warning to be extra careful not to
introduce a new
codepath that reads the variable without setting, as the compiler no
longer
helps him. If the person who touches the code is as clueless as the
compiler
and cannot follow the codepath to see the variable is never used
uninitialized,
the result will be a lot worse.
That is the reason why I do not think the idiom to squelch the compiler is
such a
good thing. Careless people touch the code, so "oe = oe" initialization
carefully
placed in the original version does not necessarily stay as a useful
documentation.
But if you use "oe = NULL" as a way to squelch the warning in the first
place, it
is no better than "oe = oe". In a sense, it is even worse, as it just
looks like any
other initialization and gives a false impression that the remainder of
the code
is written in such a way that it tolerates oe being NULL in any codepath,
or
there is some assignment before that before the code reaches places where
oe
cannot be NULL. That is different from what "oe = oe" initializaion
documents--
-in the codepath protected by "if (inline_data)", it isn't just "oe can
safely be
NULL there"; instead it is "oe can safely be *any* value there, because we
don't
use it". Of course, if you explicitly initialized oe to NULL, even if you
introduce
a codepath where oe cannot be NULL later, you won't get a warning from the
compiler, so it is no better than "oe = oe".
And that is the reason why I do not think initialization to an
innocuous-looking
random value (e.g. NULL) is a good answer, either.
When both are not good, replacing "oe = oe" with "oe = NULL" didn't make
much sense, especially when the former _could_ be used better by more
careful people. And that is the resistance J6t remembers.
But when recent compilers started to warn "oe = oe" that itself is
undefined,
the equation changed. The idiom ceased to be a way to squelch the
incorrect
compiler warning (which was the primary point of its use---the
documentation
value is secondary, as what we document is "we are squelching a false
alarm",
but we no longer are squelching anything).
See 4a5de8d (vcs-svn: avoid self-assignment in dummy initialization of
pre_off,
2012-06-01), and 58ebd98 (vcs-svn/svndiff.c: squelch false "unused"
warning
from gcc, 2012-01-27) that it updated, for an example of how we are
migrating
away from "oe = oe" idiom.
In any case, I already said initializing to 0 is OK in my previous
message, I think,
so if you are seeing warning from the compiler on uninitialized variables
in
your new code, you know you should
(1) first follow the codepath to make sure it is a false alarm (if
it isn't, fix the code);
(2) then see if you can simplify your code so that it is crystal
clear even to the stupid compiler that you are not using an
uninitialized variable; and
(3) if you can't, then initialize it to some known bad value,
avoiding the "oe = oe" idiom.
Is there anything more to discuss on this topic?
Which of the ones I told you about should get fixed?
Bye, Jojo
On 08/22/2012 06:38 PM, Joachim Schmitz wrote:
quoted
-----Original Message-----
From: Junio C Hamano [mailto:gitster@pobox.com]
Sent: Tuesday, August 21, 2012 4:06 AM
To: Joachim Schmitz
Cc: 'Johannes Sixt'; 'Jan Engelhardt'; git@vger.kernel.org
Subject: Re: git on HP NonStop
"Joachim Schmitz" [off-list ref] writes:
quoted
OK, so let's have a look at code, current git, builtin/cat-file.c,
line 196:
void *contents = contents;
This variable is set later in an if branch (if (print_contents ==
BATCH), but not in the else branch. It is later used always under the
same condition as the one under which it is set.
Apparently is is malloc_d storage (there a "free(content);"), so
there's no harm al all in initializing it with NULL, even if it only
appeases a stupid compiler.
It actually is harmful. See below.
Harmful to initialize with NULL or to use that undefined behavoir?
I checked what our compiler does here: after having warned about "vlues us
used before it is set: it actually dies seem to have initializes the value
to 0 resp. NULL.
So here there's no harm done in avoiding undefined behavior and set it to 0
resp NULL in the first place.
There is harm in tricking future programmers into thinking that the
initialization actually means something, which some of them do.
It's unlikely that you're the one to maintain that code forever, and
the "var = var" idiom is used widely within git with a clear meaning
as a hint to programmers who read a bit of git code. If they aren't
used to that idiom, they usually investigate it in the code and
pretty quickly realize that what it means.
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.
From: Andreas Ericsson [mailto:ae@op5.se]
Sent: Thursday, August 23, 2012 10:24 AM
To: Joachim Schmitz
Cc: 'Junio C Hamano'; 'Johannes Sixt'; 'Jan Engelhardt'; git@vger.kernel.org
Subject: Re: git on HP NonStop
On 08/22/2012 06:38 PM, Joachim Schmitz wrote:
quoted
quoted
-----Original Message-----
From: Junio C Hamano [mailto:gitster@pobox.com]
Sent: Tuesday, August 21, 2012 4:06 AM
To: Joachim Schmitz
Cc: 'Johannes Sixt'; 'Jan Engelhardt'; git@vger.kernel.org
Subject: Re: git on HP NonStop
"Joachim Schmitz" [off-list ref] writes:
quoted
OK, so let's have a look at code, current git, builtin/cat-file.c,
line 196:
void *contents = contents;
This variable is set later in an if branch (if (print_contents ==
BATCH), but not in the else branch. It is later used always under the
same condition as the one under which it is set.
Apparently is is malloc_d storage (there a "free(content);"), so
there's no harm al all in initializing it with NULL, even if it only
appeases a stupid compiler.
It actually is harmful. See below.
Harmful to initialize with NULL or to use that undefined behavoir?
I checked what our compiler does here: after having warned about "vlues us
used before it is set: it actually dies seem to have initializes the value
to 0 resp. NULL.
So here there's no harm done in avoiding undefined behavior and set it to 0
resp NULL in the first place.
There is harm in tricking future programmers into thinking that the
initialization actually means something, which some of them do.
Hmm, OK, I can agree to that.
It's unlikely that you're the one to maintain that code forever,
It is unlike for me to ever have to maintain this code.
Currently that's Junio's job and I won't apply for in ;-)
and the "var = var" idiom is used widely within git
This is overstating it a bit. I went thru the entire code and reported all places I could find in an earlier email.
I went back and counted: It is used in 11 files, at 15 places, for 21 variables.
OK, I may have missed a few more that were in code paths my compiler didn't see, but still some 21+ isn't really much.
with a clear meaning
Only if you call undefined behavior a 'clear meaning"!
as a hint to programmers who read a bit of git code. If they aren't
used to that idiom, they usually investigate it in the code and
pretty quickly realize that what it means.
Whether I realize what it means, is irrelevant, my compiler does not and warns about it, and as per the ANSI/ICO C standard it
invokes undefined behavior.
If a proper initialization is meaningless for these cases, don't do them at all, let the stupid compiler complain about it and the
clever programmer check whether the warning is useful, but don't avoid a compiler warning on one compiler by introducing undefined
behavior and provoke a compiler warning on another.
Bye, Jojo