RFR (S): 8214791: Consistently name gc files containing VM operations [Was: Re: RFR (S): 8214791: Rename vm_operations_g1* files to g1VMOperations*]

[Per Liden]

Looks good to me!

/Per

On 12/06/2018 01:56 PM, Thomas Schatzl wrote:
> Hi all,
> 
> On Thu, 2018-12-06 at 08:01 +1000, David Holmes wrote:
>> Hi Thomas,
>>
>> I like the turn this has taken :)
>>
> 
> :)
> 
> So there is a new webrev at
> http://cr.openjdk.java.net/~tschatzl/8214791/webrev/ that renames all
> GC specific files containing VM operations to the pattern
> <gc>VMOperations.?pp we agreed on.
> 
> The runtime/vm_operations* files are handled in JDK-8214580 also out
> for review.
> 
> Testing:
> compilation of Oracle platforms.
> 
> Thanks,
>    Thomas
> 
>> On 6/12/2018 1:05 am, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On Wed, 2018-12-05 at 12:41 +0100, Per Liden wrote:
>>>> Hi,
>>>>
>>>
>>> [...]
>>>>>> Coleen and I talked about this, and neither of us wants yet
>>>>>> another naming convention for these files.
>>>>>> See her comment in the bug report for her
>>>>>> preference.  (vmG1Operations).  I don’t care that much
>>>>>> about the specifics, so long as there is consistency (which
>>>>>> there
>>>>>> admittedly is not, at present, but
>>>>>> this proposed change just makes that worse).
>>>>>>
>>>>>
>>>>> I could rename the other gc files too to make them consistent,
>>>>> e.g.
>>>>> cmsVMOperations :)
>>>>>
>>>>> However I can be convinced to go with vmG1Operations although
>>>>> it
>>>>> does read backwards imho. At least it gets rid of the
>>>>> underscore.
>>>>>
>>>>> New webrev with that latter change:
>>>>> http://cr.openjdk.java.net/~tschatzl/8214791/webrev.1
>>>>
>>>> I don't know... I would suggest we have either a prefix or a
>>>> suffix.
>>>> The "midfix" (is that a word?) style is just strange.
>>
>> "infix" is the word. Yes infix is just plain weird here. :)
>>
>>>> For example:
>>>>
>>>>     runtime/vmOperations.hpp
>>>>     gc/shared/gcVMOperations.hpp
>>>>     gc/g1/g1VMOperations.hpp
>>>>     gc/parallel/psVMOperations.hpp
>>>>     gc/cms/cmsVMOperations.hpp
>>
>> Much better.
>>
>> I'm assuming there are reasons we have to repeat the "name" part in
>> the
>> file name even though it's uniquely determined by the directory
>> path?
>> Afterall:
>>
>> #include <g1/g1Foo.hpp>
>>
>> does look somewhat redundant versus
>>
>> #include <g1/foo.hpp>
>>
>>> I would prefer this (this is actually the original suggestion) and
>>> will
>>> do the work if everyone agrees on that. Widening to hotspot-
>>> runtime-dev
>>> in the hope that we can catch all that discussion there too then.
>>>
>>>> or maybe (to follow the vmStructs style):
>>>>
>>>>     runtime/vmOperations.hpp
>>>>     gc/shared/vmOperations_gc.hpp
>>>>     gc/g1/vmOperations_g1.hpp
>>>>     gc/parallel/vmOperations_ps.hpp
>>>>     gc/cms/vmOperations_cms.hpp
>>
>> This works for me too - I have no prejudices against underscores :)
>>
>>>>
>>>> But then I also don't think there is any real value in always
>>>> having
>>>> VM_Operation (or other things like OopClosure) classes declared
>>>> in
>>>> separate files, like we currently do. In some cases such classes
>>>> have very little in common and are better place in the context
>>>> they
>>>> are used.
>>>> For example, in ZGC the VM_ZOperation class is an internal part
>>>> of
>>>> the ZDriver, and not declared in a separate file.
>>>
>>> I agree, I saw that and think it is nice(r) :) However e.g. the
>>> g1CollectedHeap.cpp file is big enough as it is, and the files are
>>> there already.
>>
>> Probably a case by case concern. I certainly don't see a need to
>> always
>> factor out the vm-ops, but if you want all the different GC's to have
>> a
>> consistent file naming/structure then its fine.
>>
>>> It's just that every time I look at these names in the file browser
>>> I
>>> see these very strangely named files and think to myself that this
>>> should be fixed. Now here we are...
>>
>> I feel grateful that I rarely have to delve into a gc subdirectory ;-
>> )
>>
>> Thanks,
>> David
>>
>>> Thanks,
>>>     Thomas
>>>
>>>
> 
>