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

[David Holmes]

Hi Kim,

The now GC changes look fine (as did the original GC change). I'll leave 
the revised GC change to the GC folk.

Thanks,
David

On 9/04/2018 4:55 PM, Kim Barrett wrote:
>> 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/
> 
>