> -----Original Message-----
> From: Junio C Hamano [mailto:gits...@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" <j...@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.

> > 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,
> >                                               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
> 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
> 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
> good thing to do when you see compiler warnings.
> If the compiler suspects the variable might be unused, you should always
> 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
> 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
> 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
> helps him.  If the person who touches the code is as clueless as the
> and cannot follow the codepath to see the variable is never used
> 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
> 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,
> there is some assignment before that before the code reaches places where
> cannot be NULL.  That is different from what "oe = oe" initializaion
> -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
> use it".  Of course, if you explicitly initialized oe to NULL, even if you
> 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
> 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
> the equation changed.  The idiom ceased to be a way to squelch the
> compiler warning (which was the primary point of its use---the
> value is secondary, as what we document is "we are squelching a false
> but we no longer are squelching anything).
> See 4a5de8d (vcs-svn: avoid self-assignment in dummy initialization of
> 2012-06-01), and 58ebd98 (vcs-svn/svndiff.c: squelch false "unused"
> from gcc, 2012-01-27) that it updated, for an example of how we are
> 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
> 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

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to