RE: git on HP NonStop

-----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