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

[Thomas Schatzl]

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
> > 
> >