review request (S): 8042255 make gc src file exclusion more automatic

[Ron Durbin]

John

I approve the change and thanks for the explanation of
the intended functionality.

Ron Durbin


> -----Original Message-----
> From: John Coomes
> Sent: Saturday, May 17, 2014 12:56 AM
> To: David Holmes
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: review request (S): 8042255 make gc src file exclusion more automatic
> 
> David Holmes (david.holmes at oracle.com) wrote:
> > Hi John,
> >
> > On 14/05/2014 7:34 AM, John Coomes wrote:
> > > John Coomes (John.Coomes at oracle.com) wrote:
> > >> Please review this small makefile change to reduce the need to update
> > >> the list of files in make/excludeSrc.make.
> >
> > They will still need to know whether a new file is an excluded file or a
> > kept file, and possibly update the gc_shared_keep list, but at least an
> > excluded but needed file should result in a build error; whereas a
> > not-excluded unneeded file may not.
> 
> Hi David,
> 
> Thanks for looking at it.
> 
> Yes, you're exactly right; I found 4 files in gc_implementation/shared
> that were unneeded but not excluded.  At least all cms, g1, parallel
> scavenge and parnew files will be excluded automatically.
> 
> > Is it worth considering splitting the shared directory in two?
> 
> I thought about it, but only for a moment :-).  I think we should see
> how often excludeSrc.make has to be updated in jdk9 after this change;
> probably won't happen much.
> 
> > >>
> > >> http://cr.openjdk.java.net/~jcoomes/8042255-gc-exclude/
> > >
> > > A couple of people on the gc team have looked at these changes and
> > > don't object, but they also requested that someone with real makefile
> > > expertise take a look.  Anyone here willing to look at it?  Only one
> > > file changed.
> >
> > This looks okay - assuming the list of excluded files is the same before
> > and after.
> 
> There are 4 files that are newly-excluded (see the comments in the
> webrev), but they should be excluded.
> 
> > These:
> >
> >   90       gc_shared_all = $(notdir $(wildcard $(gc_impl)/shared/*.cpp))
> >   91       gc_shared_keep =                                        \
> >
> > might be slightly more efficient if defined using :=
> >
> > For that matter it might be more efficient to change:
> >
> >    82       Src_Files_EXCLUDE +=
> >        \
> >    83         $(notdir $(wildcard $(gc_impl)/concurrentMarkSweep/*.cpp))
> >       \
> >    84         $(notdir $(wildcard $(gc_impl)/g1/*.cpp))
> >        \
> >    85         $(notdir $(wildcard $(gc_impl)/parallelScavenge/*.cpp))
> >        \
> >    86         $(notdir $(wildcard $(gc_impl)/parNew/*.cpp))
> >
> > to
> >
> >    82       gc_files_exclude :=
> >       \
> >    83         $(notdir $(wildcard $(gc_impl)/concurrentMarkSweep/*.cpp))
> >       \
> >    84         $(notdir $(wildcard $(gc_impl)/g1/*.cpp))
> >        \
> >    85         $(notdir $(wildcard $(gc_impl)/parallelScavenge/*.cpp))
> >        \
> >    86         $(notdir $(wildcard $(gc_impl)/parNew/*.cpp))
> >
> >    87 Src_Files_EXCLUDE += gc_files_exclude
> 
> Sure, I like that idea and will change to use "simply-expanded" vars.
> 
> -John