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

[Thomas Schatzl]

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.

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

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

> 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