RFR(M): 8068883: Remove disabling of warning "C4355: 'this' : used in base member initializer list"

[Kim Barrett]

On Jan 13, 2015, at 1:04 PM, Volker Simonis <volker.simonis at gmail.com> wrote:
> 
> so here comes the change which re-enables the warning "C4355: 'this' :
> used in base member initializer list".
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2015/8068883/
> https://bugs.openjdk.java.net/browse/JDK-8068883
> 
> This was previously discussed in the mail thread for "RFR(S): 8068739:
> G1CollectorPolicy uses uninitialized field '_sigma' in the
> constructor" (http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-January/011743.html).
> 
> There are several places in the GC coding which disable the warning
> Visual Studio warning "C4355: 'this' : used in base member initializer
> list":
> 
> […]
> 
> However this warning is reasonable and we've recently found a bug
> which was caused by the usage of the incompletely initialized this
> pointer in the initializer list (see issue 8068739 -
> https://bugs.openjdk.java.net/browse/JDK-8068739).
> 
> Now that I've done this change I must confess that I'm not that
> enthusiastic about it any more. […]
> 
> Unfortunately, moving the corresponding field initializations from the
> initializer list into the constructor body, required the creation of
> several default constructors which is not that nice. On the other
> side, I think the code got cleaner now and we are getting rid of the
> pragmas which disabled the corresponding compiler warnings on Windows.
> Notice that although the current implementation wasn't wrong, it still
> handed over the this pointer from the initializer list several levels
> of indirection down which makes it hard to detect from a first glance
> if that usage is safe or not. But please judge yourself if it's worth
> while doing this clean-up or not.

Do we know whether relevant versions of the (Microsoft?) compiler
actually issues the warning in question for the code under
consideration? Looking at a couple of the cases under consideration
here, I don't think warnings should be generated, based on the
documentation for that warning.

Of course, it might be that the warning documentation and
implementation don't match, or I'm looking at the wrong documentation,
or I'm not understanding it properly.  Here's what I'm looking at:
http://msdn.microsoft.com/en-us/library/3c594ae3.aspx.

In general, I don't like these sorts of changes. I think the code in
question is (probably) valid, and that a heuristic of "use of
<tt>this</tt> in <em>ctor-initializers</em> are invalid" is overly
restrictive; the standard only says they are invalid before the base
class initializers are complete.

I think problems like https://bugs.openjdk.java.net/browse/JDK-8068739
can (and ought to) be detected via static analysis tools. (I know I've
seen defect reports for this sort of thing from applying such tools to
other code bases in previous jobs.) It is good that said bug was
discovered and addressed (though I'm not entirely happy with the
proposed fix, but didn't get around to commenting before it was
approved and pushed). But that it wasn't already known would seem to
me to indicate a defect in our use of static analysis tools, or in the
static analysis tools we're using.

I think applying the kinds of transformations proposed here is not an
improvement, particularly when it involves adding default constructors
that leave an object in a "partially constructed" state that requires
later (assignment-based) reconstruction or separate post-construction
initialization. See, for example, the addition of a default
CMMarkStack() constructor in order to allow ConcurrentMark::_markStack
to be initialized in the constructor body after being (partially)
constructed via default construction in the initializer list.


> I've additionally also removed the Visual Studio compiler macro which
> disable the warning "C4786:'identifier' : identifier was truncated to
> 'number' characters in the debug information." because as far as I
> could see, this was only relevant up to Visual Studio 6.0 and I hope
> nobody will ever compile the HotSpot with that compiler any more(see
> http://support.microsoft.com/kb/195386).

This seems like a worthwhile change, independent of the other changes
being proposed.  Could this be separated out.