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

[David Holmes]

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.

Thanks,
David

> 
> /Patric
>