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

[David Holmes]

Thanks John this looks good to me.

I tested out the missing 4 excluded files too - thanks for finding 
those. If the whole change can't be backported to 8u I'll look at 
getting the exclude list fixed.

David

On 17/05/2014 4:56 PM, John Coomes wrote:
> 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
>