RFR: 8200550: Xcode 9.3 produce warning on heapRegionSet.hpp

[Kim Barrett]

> On Apr 7, 2018, at 5:24 PM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi,
> 
> On Sat, 2018-04-07 at 15:48 -0400, Kim Barrett wrote:
>>> On Apr 7, 2018, at 7:03 AM, Thomas Schatzl <thomas.schatzl at oracle.c
>>> om> wrote:
>>> 
>>> Hi Kim,
>>> 
>>> On Fri, 2018-04-06 at 19:55 -0400, Kim Barrett wrote:
>>>> Please review this fix of a couple of macro definitions that have
>>>> "defined" expressions in the expansion.  Such code has undefined
>>>> behavior: C++03 16.1/3.  Xcode 9.3 warns about such code.
>>>> 
>>>> The fix for HEAP_REGION_SET_FORCE_VERIFY is a straight-forward
>>>> change to use #ifdef ASSERT to define it appropriately.
>>> 
>>> It would imho be much nicer to remove the define and use the
>>> available NOT_DEBUG_RETURN/#ifndef ASSERT macros to accomplish the
>>> same thing. (I would also be fine with using
>>> NOT_PRODUCT_RETURN/#ifndef
>>> PRODUCT).
>> 
>> That doesn’t accomplish the same thing.  The present code allows one
>> to explicitly define the macro on the (make) command line to override
>> the default behavior (based on ASSERT).  Are you suggesting removing
>> that capability?
> 
> I have never anyone seen anyone using extra defines on the command line
>  and I am very certain I have never used them myselves, but maybe it
> is common to do for others.
> I can imagine people using them for initial development, and then
> leaving them in "just in case", forgetting them rather quickly. This
> and others are certainly not documented anywhere.
> 
> It is also really easy to forget to remember to always specify the
> correct defines, i.e. be sure that the build actually contains this
> code too.
> 
> That particular define has been introduced in 2011 and like many other 
> defines/paranoia checks/* in G1 code we removed over time this one
> seems to duplicate the suggested (PRODUCT_RETURN / NOT_DEBUG_RETURN)
> functionality.
> 
> (Actually tbh, I have heard of HEAP_REGION_SET_FORCE_VERIFY for the
> very first time. The heapRegionSet* files are very old parts of G1).
> 
> Maybe Tony can comment.

I'm not going to defend it.  I don't much like this kind of littering
of the code base either.  I was trying to fix a build problem on some
(not (yet) supported by Oracle) platforms, without making a judgement
about the feature.  But I'm happy to remove HEAP_REGION_SET_FORCE_VERIFY
instead.

>> NOT_PRODUCT/#ifndef PRODUCT is the wrong choice, being contrary to to
>> the intent of an optimize build to be similar in performance
>> characteristics to a product build.
> 
> In my understanding there is an additional clause in that sentence:
> "[...] to a product build, including some extra verification".
> 
> There is lots of similar verification that uses PRODUCT_RETURN/#ifndef
> PRODUCT that also already changes performance characteristics for an
> optimized build.
> See the other PRODUCT_RETURN uses (I can find 107 in just the gc
> directory) in this and other files.

That's not my understanding from discussions with John Masamitsu (who
was the only person I knew of to regularly use and care about
"optimized" builds).  But it is certainly true there is a lot of code
that violates that rule.  I occasionally wonder whether anyone would
care or notice if we killed off "optimized" builds; we don't test that
mode, with the result that it is often broken.

> So yes, I recommend removing this capability in this instance. This
> verification does not seem to be any different than any other.
> 
> Not sure if this capability is too expensive or not (this is typically
> a bit fuzzy), but in the worst case one can use NOT_DEBUG_RETURN.

Since it was previously only under ASSERT, I'm leaving it that way.

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8200550/open.01/
incr: http://cr.openjdk.java.net/~kbarrett/8200550/open.01.inc/