RFC: Organising .inline.hpp and .hpp includes

[David Holmes]

Hi Stefan,

On 11/08/2016 6:21 PM, Stefan Karlsson wrote:
> Hi David,
>
>
> On 2016-08-11 09:31, David Holmes wrote:
>> I'm trying to tackle the incorrect inclusion of .hpp files instead of
>> .inline.hpp starting with the use of atomic.inline.hpp ref:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8157907
>>
>> The basic rules would seem to be:
>>
>> If class Foo has inline methods that need to call the inline Atomic
>> methods then:
>>
>> - Those methods should be defined in foo.inline.hpp
>>
>>    inline void Foo::a() { /* do stuff with atomics */ }
>>
>> - foo.hpp declares those methods:
>>
>>    inline void a();
>>
>> - foo.inline.hpp includes foo.hpp
>> - foo.inline.hpp includes atomic.inline.hpp
>> - foo.cpp includes foo.inline.hpp
>>
>> Do have agreement so far?
>
> Yes.

Good start :)

>>
>> Now suppose we have class Bar which uses Foo.
>>
>> - bar.cpp should include foo.inline.hpp
>> - If bar.hpp only references Foo but doesn't need to see a full
>> definition then bar.hpp can forward declare Foo
>> - If bar.hpp defines inline methods that require full access to Foo
>> then either:
>>
>> a) bar.hpp includes foo.inline.hpp; or
>> b) The inline functions of Bar are themselves moved to bar.inline.hpp
>> which includes foo.inline.hpp
>>
>> I think there was general opinion that (a) is undesirable, and that
>> (b) is preferable - is that right?
>
> Yes.
>
> Alternatively, if inlining isn't needed, place these functions in bar.cpp.

Right - though it is effectively impossible to tell whether inlining is 
"needed" - there is a tendency to define "small" methods as inline 
regardless of need.

>>
>> What I am finding is that the fan out of applying (b) is rather large
>> - to the point where we may end up having x.hpp, x.inline.hpp and
>> x.cpp variants for very many x!
>
> Yes, that's what I've also seen while investigating:
>
> https://bugs.openjdk.java.net/browse/JDK-8154079
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-April/022626.html

So I see that you basically avoided the explosive fan out by getting rid 
of the inline functions.

I think I missed when this was discussed. :( It completely subsumes what 
I'm trying to fix with atomic.hpp.

> I have an updated version that works with and without precompiled headers:
> http://cr.openjdk.java.net/~stefank/8154079/webrev.02/
>
> Regarding atomic.inline.hpp and the atomic_<os_cpu>.inline.hpp files,
> these files typically don't have a lot of dependencies on other header
> files and a pragmatic way forward could be to even further minimize
> those dependencies and move the implementation into atomic_<os_cpu>.hpp
> files and get rid of atomic.inline.hpp. I know that goes against the
> guidelines mentioned above, but it's an alternative to use if the fan
> out becomes too large.

Would be simpler I think to get rid of atomic.inline.hpp and place those 
definitions in atomic.hpp. But that is going against the guidelines 
completely - and there is nothing special about atomic.inline.hpp. The 
main issue to address is avoiding compilation errors because the .hpp 
was included and not the .inline.hpp.

I'm not sure we have a tractable problem at this stage. The simplest fix 
is to not define the various inline functions, at least at the 
second-level (so assume something like atomic.inline.hpp is first-level 
and important, but any .hpp's that need it are "fixed" by un-inlining 
their functions and moving to .cpp file). That removes the fan out, at 
the loss of inlining at the second level.

>> Just to add another dimension to this, what should precompiled.hpp
>> include? I would assume the .inline.hpp files right?
>
> I would prefer if we got rid of the .inline.hpp files from precompiled.hpp.

Okay ... not sure what that really means from a precompilation 
perspective ... but if it works, okay.

So how to proceed? :)

Cheers,
David

> Thanks,
> StefanK
>
>>
>> Thanks,
>> David
>