RFR (XS): 8145674: Fix includes and forward declarations in g1Remset files

[Volker Simonis]

On Fri, Dec 18, 2015 at 12:35 PM, Thomas Schatzl
<thomas.schatzl at oracle.com> wrote:
> Hi Volker,
>
>   thanks for your comments.
>
> On Fri, 2015-12-18 at 12:22 +0100, Volker Simonis wrote:
>> Hi Thomas,
>>
>> I like such kind of changes, nevertheless I have some comments:
>>
>> in g1RemSet.hpp the following classes:
>>
>> G1BlockOffsetSharedArray
>> G1SATBCardTableModRefBS
>
> Both used in the ScanRSClosure class.

Sorry, you're right. I accidentally checked in an older hs-comp
repository where '8142390: Move ScanRSClosure to header file' wasn't
pushed.

>
>> outputStream
>>
>> are not used or referenced, so there's no need to forward declare them.
>
> I will remove that one. Sorry, overlooked that one.
>
>> If files which include g1RemSet.hpp need them, than they should
>> forward declare the classes (and only the classes) they really require
>> themselves. There's no need to provide forward declarations in a
>> header file to fulfill requirements of users of that header file.
>
> That has not been the intent.
>
>> You added an include of "gc/g1/heapRegion.hpp" to g1RemSet.hpp but
>> also a forward declaration for "class HeapRegion". So either the
>> forward declaration of HeapRegion is enough to fulfil all the
>> requirements in heapRegion.hpp in which case you could remove the
>> include or the include is really needed (why?) in which case you can
>> remove the forward declaration because heapRegion.hpp defines
>> HeapReagion. From a first look it seems you are only using HeapRegion
>> pointers in g1RemSet.hpp so you should probably drop the inclusion of
>> heapRegion.hpp.
>
> The file has ScanRSClosure that inherits from HeapRegionClosure that is
> also in heapRegion.hpp. I would assume that inheriting from something
> needs its definition (i.e. a forward declaration is not sufficient), no?
>

You're right (same reason as above).

> I did not remove the HeapRegion* forward declarations after adding
> heapRegion.hpp.
>

I suppose you wanted to say "I did remove...". At least that's what
the new webrev says.

Why did you had to add "memory/allocation.hpp" now? Was this intentional ?

Otherwise looks good.

Regards,
Volker

>> The other extra includes are fine.
>>
>> The inclusion of "gc/g1/heapRegion.inline.hpp" into g1RemSet.cpp is
>> also fine because g1RemSet.cpp uses the inline function
>> HeapRegion::in_collection_set() which is defined in
>> "gc/g1/heapRegion.inline.hpp"
>>
>> Regards,
>> Volker
>>
>>
>> On Fri, Dec 18, 2015 at 11:48 AM, Thomas Schatzl
>> <thomas.schatzl at oracle.com> wrote:
>> > Hi all,
>> >
>> >   can I have reviews for this change that adds necessary includes and
>> > forward declarations in g1RemSet.?pp files? While it compiles right now
>> > as is, if something changes in this area, compilation tends to break in
>> > interesting ways.
>> >
>> > CR:
>> > https://bugs.openjdk.java.net/browse/JDK-8145674
>> > Webrev:
>> > http://cr.openjdk.java.net/~tschatzl/8145674/webrev/
>> > Testing:
>> > jprt
>
> New webrevs (pending another jprt run):
>
> http://cr.openjdk.java.net/~tschatzl/8145674/webrev.1 (full)
> http://cr.openjdk.java.net/~tschatzl/8145674/webrev.0_to_1 (diff)
>
>
> Thanks,
>   Thomas
>
>