6348631 - request for review (updated)

[Volker Simonis]

On Fri, Dec 3, 2010 at 4:10 AM, David Holmes <David.Holmes at oracle.com> wrote:
> Y. S. Ramakrishna said the following on 12/03/10 03:10:
>>
>> On 12/02/10 02:22, Volker Simonis wrote:
>>>
>>> As far as I remember, the rule of thumb (as detailed in includeDB_core)
>>> was:
>>> - put declarations into .hpp
>>> - put definitions of inline functions into .inline.hpp
>>> - .hpp files only include other .hpp files
>>> - every xxx.inline.hpp includes the corresponding xxx.hpp
>
> Seems the wrong way round to me but let's move on.
>

This whole dance with .hpp and .inline.hpp was done to avoid include
cycles. The root cause for the problem is that C/C++ has no standard
way of finding inline functions which are not in the current
compilation unit.

So consider a class 'A' which contains an inline member function
'A::a()' which requires class 'B' for its definition and a class 'B'
with an inline member function 'B::b()' which requires class 'A' for
its definition. Now you have two possibilities:

1. You declare class 'A' in a.hpp and define 'A::a()' in a.cpp.
However with the above mentioned C++ compiler deficiencies, other
clients of 'A' which include a.hpp won't be able to inline 'A::a()'
because it's not available in their compilation unit.

2. (the current HotSpot approach) You declare class 'A' in a.hpp and
define 'A::a()' in a.inline.hpp, which needs to include a.hpp. The
same holds for 'B': you declare class 'B' in b.hpp and define 'B::b()'
in b.inline.hpp which again includes b.hpp. a.inline.hpp can now
include b.hpp and b.inline.hpp can include a.hpp.Therewith you've
broken the include cycle (at least as long as 'A::a()' doesn't want to
inline 'B::b()' in which case this approach won't work either).

The only drawback of  approach 2 (as you mentioned below) is that a
user of class 'A' needs to decide if he only needs the declaration of
'A' or also the definition of some inline function. The pragmatic
approach (and I think the current rule of thumb in HotSpot) is to
always include the .inline.hpp file if there is one and the .hpp file
otherwise.

I think the whole confusion with .inline.hpp is more or less a 'naming
issue'. In fact .inline.hpp are not real header files but more
implementation files. If they had been named '.inline.cpp' however, it
would have looked strange to include ".cpp" files into other .cpp
files.

>>> - every yyy.cpp includes xxx.inline.hpp if it requires definitions
>>> from it or just xxx.hpp if it only needs the declarations
>
> Again sounds "wrong" as the cpp has to know is inline and what isn't and
> that isn't its business. cpp should include hpp should include inline.hpp.
> But again lets move on.
>
>>> Now after we have no includeDB anymore I would also advocate for the
>>> inclusion of system headers where they are needed. So if
>>> xxx.inline.hpp requires a declaration from zzz.h, it should include
>>> it. Otherwise the users of xxx.inline.hpp (i.e. every file which
>>> includes xxx.inline.hpp) would have to manually include zzz.h BEFORE
>>> including xxx.inline.hpp which is not obvious.
>>>
>>> As mentioned by Ivan, another nice benefit of this approach is that if
>>> xxx.inline.hpp will be in the precompiled header file, so will be the
>>> system headers like zzz.h. This  will speed up compilation compared to
>>> the case where zzz.h would be directly included into  the .cpp  file.
>>
>> I agree. This sounds like the right approach, and one that is consistent
>> with past practice. This would also be exactly what the
>> make-deps removal accomplished for the existing .inline.hpp dependencies
>> as
>> well.
>
> So just to be clear here, Volker and Ramki agree with me that the system
> includes should remain in the header files that need them - right? Whereas
> Ivan and Coleen want to see them moved into the cpp files.
>

>From my side: YES.

> David
>
>