RFR (XS): 8145674: Fix includes and forward declarations in g1Remset files
volker.simonis at gmail.com
Fri Dec 18 13:52:29 UTC 2015
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:
> 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
>> 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
> 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
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.
>> 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
>> 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)
More information about the hotspot-gc-dev