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

[Patric Hedlin]

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.

Thanks David.

/Patric