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

[David Holmes]

Hi Patric,

On 12/07/2020 12:00 am, David Holmes wrote:
> On 9/07/2020 9:56 pm, Patric Hedlin wrote:
>> Hi David,
>>
>> On 2020-07-09 11:09, David Holmes wrote:
>>> Hi Patric,
>>>
>>> On 9/07/2020 5:17 am, Patric Hedlin wrote:
>>>> Dear all,
>>>>
>>>> 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.

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.

Thanks,
David

> Thanks,
> David
> 
>>
>> /Patric
>>