RFR (L, tedious) 8199809: Don't include frame.inline.hpp and other.inline.hpp from .hpp files
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Mar 22 07:16:26 UTC 2018
On Thu, Mar 22, 2018 at 7:06 AM, David Holmes <david.holmes at oracle.com>
wrote:
> On 22/03/2018 3:52 PM, Thomas Stüfe wrote:
>
>> On Wed, Mar 21, 2018 at 11:34 PM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>> <trimming>
>>
>> On 22/03/2018 1:35 AM, Thomas Stüfe wrote:
>>
>> On Wed, Mar 21, 2018 at 2:02 PM, David Holmes
>> <david.holmes at oracle.com <mailto:david.holmes at oracle.com> On
>> 21/03/2018 10:39 PM, coleen.phillimore at oracle.com
>> <mailto:coleen.phillimore at oracle.com>
>> Hm so I need to add the #include for macroAssembler.hpp
>> somewhere new like nativeInst_ppc.hpp or does just
>> removing it
>> from interpreterRT_ppc.hpp fix the problem?
>>
>>
>> Whatever code is in the included platform specific header
>> still
>> needs to ensure the definitions that it needs have been
>> included. If
>> those are shared files then you may just be able to move
>> them into
>> the shared cpp file, but any platform specific headers must
>> still be
>> included in the platform specific headers.
>>
>>
>> I disagree in this particular case. In my opinion, headers whose
>> purpose is to be included into class declarations should not
>> include other headers.
>>
>>
>> ??? If the code you are including relies on things from other header
>> files then you have no choice else it won't compile!
>>
>>
>> Is this a misunderstanding? I am not saying not to provide the
>> dependencies. But they cannot be provided from within this header, if this
>> header gets dropped in right in the middle of a class definition (like e.g.
>> os_aix.hpp), right?
>>
>> If, for an easy example, my header uses pthread_t, I cannot just simply
>> include pthread.h, because then all declarations of pthread.h appear in
>> class scope in the surrounding class. So I have to make sure
>> the platform header is included elsewhere, preferably at the start of the
>> surrounding header, or of the cpp file.
>>
>
> I would find it undesirable to have to put in a platform specific include,
> like pthread.h, if the surrounding header or cpp file were shared files.
> But you're saying that including it in place may not actually work anyway.
Yes unfortunately. The correct solution is to use C++ namespaces. Since
namespaces can have multiple bodies - as opposed to class definitions -
this whole include-into-a-class-definition pattern can go away.
Thomas
>
> David
>
>
> Thomas
>>
>>
>> David
>>
>> Thanks, Thomas
>>
>> David
>> -----
>>
>> thanks,
>> Coleen
>>
>>
>>
>> Thanks, Thomas
>>
>>
>>
>>
>>
>> On Wed, Mar 21, 2018 at 11:41 AM, Thomas Stüfe
>> <thomas.stuefe at gmail.com
>> <mailto:thomas.stuefe at gmail.com> <mailto:thomas.stuefe at gmail.com
>> <mailto:thomas.stuefe at gmail.com>>
>> <mailto:thomas.stuefe at gmail.com
>> <mailto:thomas.stuefe at gmail.com>
>>
>> <mailto:thomas.stuefe at gmail.com
>> <mailto:thomas.stuefe at gmail.com>>>> wrote:
>>
>> Hi Coleen,
>>
>> linuxs390 needs this:
>>
>> - .../source $ hg diff
>> diff -r daf3abb9031f
>> src/hotspot/cpu/s390/interpreterRT_s390.cpp
>> ---
>> a/src/hotspot/cpu/s390/interpreterRT_s390.cpp
>> Wed Mar 21
>> 08:37:04 2018 +0100
>> +++
>> b/src/hotspot/cpu/s390/interpreterRT_s390.cpp
>> Wed Mar 21
>> 11:12:03 2018 +0100
>> @@ -65,7 +65,7 @@
>> }
>>
>> // Implementation of SignatureHandlerGenerator
>> -InteprerterRuntime::Signature
>> HandlerGenerator::SignatureHandlerGenerator(
>>
>> +InterpreterRuntime::Signature
>> HandlerGenerator::SignatureHandlerGenerator(
>>
>> const methodHandle& method, CodeBuffer*
>> buffer) :
>> NativeSignatureIterator(method) {
>> _masm = new MacroAssembler(buffer);
>> _fp_arg_nr = 0;
>>
>> (typo). Otherwise it builds fine.
>>
>> I'm getting build errors on AIX which are a
>> bit more
>> complicated,
>> still looking..
>>
>> Thanks, Thomas
>>
>>
>> On Wed, Mar 21, 2018 at 1:08 AM,
>> <coleen.phillimore at oracle.com
>> <mailto:coleen.phillimore at oracle.com>
>> <mailto:coleen.phillimore at oracle.com
>> <mailto:coleen.phillimore at oracle.com>>
>> <mailto:coleen.phillimore at oracle.com
>> <mailto:coleen.phillimore at oracle.com>
>>
>> <mailto:coleen.phillimore at oracle.com
>> <mailto:coleen.phillimore at oracle.com>>>> wrote:
>>
>> Summary: Remove frame.inline.hpp,etc from
>> header
>> files and
>> adjust transitive includes.
>>
>> Tested with mach5 tier1 on Oracle
>> platforms: linux-x64,
>> solaris-sparc, windows-x64. Built with
>> open-only
>> sources
>> using --disable-precompiled-headers on
>> linux-x64,
>> built with
>> zero (also disable precompiled headers).
>> Roman
>> built with
>> aarch64, and have request to build ppc,
>> etc. (Please test
>> this patch!)
>>
>> Semi-interesting details: moved
>> SignatureHandlerGenerator
>> constructor to cpp file, moved
>> interpreter_frame_stack_direction() to
>> target
>> specific hpp
>> files (even though they're all -1),
>> pd_last_frame to
>> thread_<os_cpu>.cpp because there isn't a
>> thread_<os_cpu>.inline.hpp file, lastly
>> moved
>> InterpreterRuntime::LastFrameAccessor into
>> interpreterRuntime.cpp file, and a few other
>> functions moved
>> in shared code.
>>
>> This is the last of this include file
>> technical
>> debt cleanup
>> that I'm going to do. See bug for more
>> information.
>>
>> open webrev at
>> http://cr.openjdk.java.net/~coleenp/8199809.01/webrev
>> <http://cr.openjdk.java.net/~coleenp/8199809.01/webrev>
>> <http://cr.openjdk.java.net/~c
>> oleenp/8199809.01/webrev
>> <http://cr.openjdk.java.net/~coleenp/8199809.01/webrev>>
>> <
>> http://cr.openjdk.java.net/%7Ecoleenp/8199809.01/webrev
>> <http://cr.openjdk.java.net/%7Ecoleenp/8199809.01/webrev>
>> <http://cr.openjdk.java.net/%7
>> Ecoleenp/8199809.01/webrev
>> <http://cr.openjdk.java.net/%7Ecoleenp/8199809.01/webrev>>>
>> bug link
>> https://bugs.openjdk.java.net/browse/JDK-8199809
>> <https://bugs.openjdk.java.net/browse/JDK-8199809>
>> <https://bugs.openjdk.java.net/browse/JDK-8199809
>> <https://bugs.openjdk.java.net/browse/JDK-8199809>>
>> <https://bugs.openjdk.java.ne
>> t/browse/JDK-8199809
>> <https://bugs.openjdk.java.net/browse/JDK-8199809>
>> <https://bugs.openjdk.java.net/browse/JDK-8199809
>> <https://bugs.openjdk.java.net/browse/JDK-8199809>>>
>>
>> I'll update the copyrights when I commit.
>>
>> Thanks,
>> Coleen
>>
>>
>>
>>
>>
>>
>>
More information about the hotspot-dev
mailing list