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

[Ioi Lam]

The webrev looks OK to me.

I would suggest using a name like PreserveFlag to be consistent with 
existing classes

./share/adlc/forms.hpp:class PreserveIter {
./share/gc/serial/markSweep.hpp:class PreservedMark {
./share/gc/shared/preservedMarks.hpp:class PreservedMarks {
./share/gc/shared/preservedMarks.hpp:class PreservedMarksSet : public 
CHeapObj<mtGC> {
./share/utilities/preserveException.hpp:class PreserveExceptionMark {
./share/utilities/preserveException.hpp:class 
CautiouslyPreserveExceptionMark {
./share/utilities/preserveException.hpp:class WeakPreserveExceptionMark {
./share/opto/graphKit.hpp:class PreserveJVMState: public StackObj {
./share/opto/graphKit.hpp:class PreserveReexecuteState: public StackObj {


... especially we already have a class SaveRestore, so AutoSaveRestore 
makes it confusing.

./share/jfr/leakprofiler/utilities/saveRestore.hpp:class SaveRestore {
./share/jfr/leakprofiler/utilities/saveRestore.hpp:class 
SaveRestoreCLDClaimBits : public StackObj {

Thanks
- Ioi



On 7/13/20 8:39 PM, David Holmes wrote:
> 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