RE: git on HP NonStop

From: Joachim Schmitz <jojo@schmitz-digital.de>
Date: Aug 20, 2012, 20:51 UTC
Message-ID: <001901cd7f15$98c6ab50$ca5401f0$@schmitz-digital.de>
In-Reply-To: <7vfw7hle6l.fsf@alter.siamese.dyndns.org>

Show 28 quoted lines

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Monday, August 20, 2012 6:30 PM
> To: Johannes Sixt
> Cc: Joachim Schmitz; 'Jan Engelhardt'; git@vger.kernel.org
> Subject: Re: git on HP NonStop
> 
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
> > Am 8/20/2012 12:36, schrieb Joachim Schmitz:
> >> int var = var;
> >> char *othervar = othervar;
> >>
> >> ...
> >>
> >> What is the reason for using that self-init stuff? I don't think it
> >> is really portable, is it?
> >
> > It is used to avoid "var may be used uninitialized" warnings for some
> > compilers. Officially (according to the C standard), it is undefined
> > behavior. But I've observed considerable resistance by Junio to fix
> > this properly.
> 
> I had resisted
> 
> 	-	int foo = foo;
>         +	int foo = 0;
> 
> in the past.  If some compiler is not seeing that "foo" is never used

uninitialized,

> such a compiler will generate an unnecessary initialization, so it is not

> _proper_ fix anyway (in fact, I do not think a proper fix exists, short of
> simplifying the code so that less sophisticated compilers can see that

"foo" is

> never used uninitialized).
> 
> So, no, I never resisted a "proper" fix.  I resisted swapping an

unsatisfactory

> workaround with another.
> 
> Between the two unsatisfactory workarounds, the latter (explicit and
> unnecessary assignment to an innocuous value) is lessor of two evils, so I

do not

> particularly mind it, though.  Indeed, I think more recent history shows

that we

> have such changes.

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.

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?

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? I'm too Next one, lazy to check... I'd just set them to 0 and stop worrying.

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! It is checked for !NULL later, so it should really get initialized to NULL in the first place!

Same file, line 2437, same variable name, same story! Same file, line 2616, variable e, it is used in an if branch but set after that! Same file again, line 2917, variable oe again. Same story as above.

Next file, ll-merge.c, line
        static const struct ll_merge_options default_opts;
Somewhat different story here, compiler warning claims " const variable
"default_opts" requires an initializer"
Possible fix:
        static const struct ll_merge_options default_opts = {0};

next file, match-trees.c, line 75ff:
                const unsigned char *elem1 = elem1;
                const unsigned char *elem2 = elem2;
                const char *path1 = path1;
                const char *path2 = path2;
                unsigned mode1 = mode1;
                unsigned mode2 = mode2;
Some get set, some not, depending on code path, but all get used, with
possibly bogus content.

Next file, merge-recursive.c, line 1903:
        struct tree *mrtree = mrtree;
passed on my address to another function, which hopefully knows how to treat
it. It woult be learer and simpler to just have
        struct tree *mrtree = NULL;
wouldn't it?

Next file, run-command.c, line 272:
        int failed_errno = failed_errno;
Set deeply nested in some cases. Seems to be used reasonably, but again, why
take chanses= 0 is a goot errno ;-)

Next file, submodule.c, line 265:
        struct commit *left = left, *right = right;
As far as I can see it is not set properly before it gets used in some
cases.

Next filen, transport.c, line 106:
                int cmp = cmp, len;
I seem to see code paths whet it is used without being set properly

Next file, vcs-svn/svndiff.c, line 299.... oh, that one has been fixed and initialized to -1.

Next (and last) file, wt-status.c, line 267:
        int status = status;
It apparently does get set properly before use. So here it once may once
again just make a compiler happy to set it to 0.

Bye, Jojo