RFR(M): 8245226: Clean-up FlagSetting and remove misuse.

[David Holmes]

Hi Patric,

On 14/07/2020 12:49 am, Patric Hedlin wrote:
> Hi David
> 
> On 2020-07-13 02:43, David Holmes wrote:
>> Hi Patric,
>>
>> < . . . >
>>>>>> I would like to ask for help to review the following change/update:
>>>>>>
>>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8245226
>>>>>> Webrev: http://cr.openjdk.java.net/~phedlin/tr8245226/
>>>>>>
>>>>>>
>>>>>> FlagSetting is sometimes used as a general mechanism for local 
>>>>>> save/modify and restore. This is not the intention (it should work 
>>>>>> as a red-flag when modifying debug options). Instead, use a 
>>>>>> general mechanism in these cases (introduced in here), and 
>>>>>> preserve FlagSetting for its intended purpose (including clean-up).
>>>>>
>>>>> Sorry but I'm not seeing why FlagSetting can't be used as a general 
>>>>> mechanism even though it is described as being for debug flags. 
>>>>> Introducing a similar mechanism seems redundant to me.
>>>>>
>>>>
>>>> Let me ask you this. How well do you think 'FlagSetting' describes 
>>>> the actual operation performed in the general use-case? Is it more 
>>>> clear than explicitly stating that you intend a save/modify and a 
>>>> restore in the local context? It is not as much about if it can be 
>>>> used as it is about if it should be used. The use of 'FlagSetting' 
>>>> and friends should be moved aside, given a new name (signalling the 
>>>> thin ice you are entering) and restricted to use on admissible 
>>>> flag/options only. I'm assuming you agree that modifying global 
>>>> options might have undesirable effects. But that can wait until 
>>>> after, or be part of, the rework. This part is about separating the 
>>>> use-cases. If you feel strongly about preserving 'FlagSetting' (and 
>>>> friends), please make your argument based on the merits of the 
>>>> current concept and implementation (such as the use of memcpy), not 
>>>> a dismissive "I don't see the point".
>>>
>>> Didn't intend to be dismissive, but if there is a bigger picture here 
>>> then you needed to explain it - as you now have. I agree as a general 
>>> mechanism FlagSetting is misnamed. So if the intent is to set it 
>>> aside as a second step then that seems reasonable.
>>
>> I don't know what I thought I saw when I initially looked at this, but 
>> what I see now is not what I thought. My apologies for that.
>>
> No need David. I just want the sharper end of your reviews.
> 
>> I'm not sure that defining FLAG_GUARD purely as a means to introduce a 
>> temporary variable name that isn't even visible to the reader, serves 
>> a useful purpose. If it can't retain its existing functionality of 
>> being a full declaration then I would suggest just dropping it.
> 
> I agree it should be removed, but at a later time, with the rest of the 
> file. After (at least) a little thought, I decided to let it remain in 
> the code as a "warning", of use both brittle and ugly. Its use leads 
> back to "flagSetting.hpp" (as do *FlagSetting), with a documented legacy 
> purpose and it identifies (if nothing else) a number of "suspects". I 
> will simply add a clarifying comment.

Given it is only used in test code and given you have already changed 
things like:

FLAG_GUARD(MinHeapSize);

to

AutoSaveRestore<size_t> FLAG_GUARD(MinHeapSize);

it seems a simple step to just remove FLAG_GUARD altogether:

AutoSaveRestore<size_t> _guard1(MinHeapSize);

But your call.

Thanks,
David

> 
> Thanks David.
> 
> /Patric