RFR(M): 8038498: Fix includes and C inlining after 8035330

[Stefan Karlsson]

On 2014-04-01 08:02, David Holmes wrote:
> Hi Goetz,
>
> Not a review ...
>
> On 31/03/2014 10:42 PM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> could I please get reviews for this change?  I please also need a 
>> sponsor.
>> I would like to get this into 8u20, as 8036330 was downported, too.
>>
>> I'd like to make the point of this change clear once more, independent
>> of which compiler does what.
>>
>> I think we agree that basically
>>    - a file should include all headers that define symbols or include
>>      method bodies used in that file
>
> Personally I don't see why a cpp file should have to be aware that a 
> member function it uses just happens to have been defined in an 
> inline.hpp file by its author. Seems to break encapsulation to me. :(

Yes, it's unfortunate, but necessary to prevent problems like circular 
dependencies. If we don't follow it, we not only risk breaking the code 
with the offending patch, but future patches might be harder to create 
because of the transitive closure of include files that is introduced by 
including the .inline.hpp file.
>
>>    - a .hpp header should not include a .inline.hpp header
>
> I see 277 occurrences of this in the existing OpenJDK hotspot code, so 
> I don't think we can take this as a general rule either.
>
> Note I'm not saying this fix isn't right or needed, just that the 
> "rules" you are trying to embody don't seem to generally apply.

This is a rule I try hard to enforce when reviewing changes. If you 
don't follow it we'll end up with problems like this.

I think it's worth adding to:
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide

thanks,
StefanK

>
> David
> -----
>
>> This is violated
>>
>> 1.)    by defining
>>   template <class T> void immediate_rs_update(HeapRegion* from, T* p, 
>> int tid) {
>>     if (!from->is_survivor()) {
>>        _g1_rem->par_write_ref(from, p, tid);
>>      }
>> }
>>   in g1CollectedHeap.hpp, as par_write_ref in declared 'inline' and 
>> only defined in
>>      g1RemSet.inline.hpp - which is not included and should not be 
>> included
>>      in g1CollectedHeap.hpp.
>> 2. by including g1CollectedHeap.inline.hpp in sparsePRT.hpp
>>
>> This change fixes these two points by moving functions into 
>> .inline.hpp files
>> and fixing the include.
>>
>> Best regards,
>>    Goetz.
>>
>>
>>
>>
>> From: Lindenmaier, Goetz
>> Sent: Donnerstag, 27. März 2014 17:46
>> To: hotspot-dev at openjdk.java.net; hotspot-gc-dev at openjdk.java.net
>> Subject: RFR(M): 8038498: Fix includes and C inlining after 8035330
>>
>> Hi,
>>
>> Please review and test this change.  I please need a sponsor:
>> http://cr.openjdk.java.net/~goetz/webrevs/8038498-incl/
>>
>> Change 8035330: Remove G1ParScanPartialArrayClosure and 
>> G1ParScanHeapEvacClosure
>> broke the dbg build on AIX. That's because do_oop_partial_array() is 
>> added in
>> a header, but requires inline function par_write_ref() through 
>> several inlined
>> calls. In some cpp files, like arguments.cpp, par_write_ref() is not 
>> defined
>> as the corresponding inline header is not included.  The aix debug VM 
>> does not start.
>>
>> This can be solved by including g1RemSet.inline.hpp in 
>> g1CollectedHeap.inline.hpp.
>>
>> Unfortunately this now causes a cyclic dependency that breaks the 
>> linux build.
>> A inline.hpp file is included ahead of a .hpp file, so that the code 
>> in the inline.hpp file
>> can not see the class declaration.
>> This is caused because g1CollectedHeap.inline.hpp is included in 
>> sparsePRT.hpp.
>> But .inline.hpp files never should be included in .hpp files.
>>
>> To resolve this, I changed this inlcude to g1CollectedHeap.hpp. As 
>> consequence,
>> I had to move a row of functions to existing .inline.hpp files.
>>
>> I did debug, fastdebug and product builds on linux_ppc64, aix_ppc64,
>> sun_64, bsd_x86_64 and linux_x86_64 and tested that the VM starts up.
>>
>> Best regards,
>>    Goetz
>>