RE: git on HP NonStop
- From
Joachim Schmitz <jojo@schmitz-digital.de>
- Date
- Aug 22, 2012, 16:38 UTC
- Message-ID
- <002b01cd8084$8459abc0$8d0d0340$@schmitz-digital.de>
- In-Reply-To
- <7vy5l9f186.fsf@alter.siamese.dyndns.org>
Show 21 quoted lines
> -----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" <jojo@schmitz-digital.de> 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.
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.
Show 13 quoted lines
> > 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,
Show 6 quoted lines
> > 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
Show 9 quoted lines
> 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
Show 7 quoted lines
> 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
Show 7 quoted lines
> 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
Show 13 quoted lines
> 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