[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR(M): 8068883: Remove disabling of warning "C4355: 'this' : used in base member initializer li
From:       Kim Barrett <kim.barrett () oracle ! com>
Date:       2015-01-14 20:50:27
Message-ID: 0459695D-FA49-4233-BE79-EBEE83720FC7 () oracle ! com
[Download RAW message or body]

On Jan 14, 2015, at 11:28 AM, Volker Simonis <volker.simonis@gmail.com> wrote:
> 
> Nevertheless, I won't be
> upset if we decide to leave the code as it stands today as long as we
> brought up and discussed all the different aspects of the problem.

Thanks for that. I'm sure we've all been in the position of doing a
bunch of work, only to have someone suggest that maybe something quite
different might be preferred.

> On Wed, Jan 14, 2015 at 1:26 AM, Kim Barrett <kim.barrett@oracle.com> wrote:
>> On Jan 13, 2015, at 1:04 PM, Volker Simonis <volker.simonis@gmail.com> wrote:
> Yes, the warning is still generated with VS 10. Just remove the pragma

Thanks.  That's what I expected, but I'm not in a position to easily
perform the test.  I doubt it has changed with more recent versions of
the compiler either.

>> 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.
>> […]
>> Here's what I'm looking at:
>> http://msdn.microsoft.com/en-us/library/3c594ae3.aspx.
> 
> You're looking at the right place.

Looking back at the Microsoft documentation again, it seems I'd
forgotten what that page said between when I read it and when I wrote
my review response. In particular, I re-read the relevant part of the
standard, and thereby confused myself about what the Microsoft
documentation says. The Microsoft documentation makes several
statements that I believe are inaccurate according to the standard,
and as a result I think the warning may be issued for
standards-conforming code, e.g. the warning is subject to (many) false
positives. [I can provide details if someone is really interested.]

Of course, compilers are permitted to issue warnings for code that is
standards-conforming but thought to be (potentially) problematic.

Note that the Point class example you (Volker) give is a different
source of undefined behavior, and isn't specifically about
constructors.

> I agree that static analysis tools should be able to detect such
> errors. But I'm not aware of any of these tools being run regularly on
> every HotSpot check-in.

Not that I know of :-( But I do know that static analysis tools are
being applied to the Hotspot code base with some frequency; bug
reports occasionally come in that are either clearly marked as such
(web search for "openjdk parfait" for some examples), or can be
inferred from the description as probably being from such tools. Thus
my disappointment that 8068739 wasn't a known bug (that could have
been fixed a long time ago).

> The compiler can definitely not detect such errors because in the
> general case it hands over the not fully initialized 'this' pointer to
> a constructor of which the implementation is in a different
> compilation unit. So a warning is the best he can do.

Whether this is "the best that he [the compiler] can do" depends a lot
on one's opinion's regarding false positives.

> While I also think that the code in question here is correct today,
> I'd consider it at least hard to understand and error prone. 
> […]
> I agree that the proposed workaround isn't ideal either, but I think
> it is safer.

Any code with ordering constraints related to side effects is going to
be hard to understand and error prone. That's one of the reasons to
avoid side-effects. And while member initializations are technically
side-effects, I think using them ends up being beneficial far more
often than not.

Any "solution" that involves the introduction of constructors that
only "partially" construct the object and require later additional
setup or initialization is not appealing to me. Ideally, objects
should provide their full functionality once constructed; anything
else is hard to reason about and often leads to the proliferation of
"if (!fully_initialized) ..." and "assert(fully_initialized)" code
blobs, or to bugs due to the lack of such code blobs.

In the specific case of 8067739, part of the problem is that the
initialization of _sigma is more complicated than it ought to be (to
be fixed by new https://bugs.openjdk.java.net/browse/JDK-8068942).
The initialization of _sigma was also poorly placed.  (Though even
moving the _sigma initialization into the ctor-initializer wouldn't
have avoided 8067739, due to the ordering of the relevant members.)

>>> 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.
>> 
> 
> Sure, if we decide not to push this patch we can still just remove
> that pragma in a separate change.

For ease of review, regression tracking, and software archeology via
VCS, it's generally beneficial to keep unrelated changes separate.
About the only person who (sometimes) benefits from smooshing them
together is the proposer of the changes. [In other words, while I
recognize it might be more work for you, my preference would be that
they be separated even if both are ultimately approved. I don't know
how others feel about this sort of thing..  If you (Volker) don't want
to deal with the extra overhead, I'll take care of it.]

[prev in list] [next in list] [prev in thread] [next in thread]