RFR (L, tedious) 8199809: Don't include frame.inline.hpp and other.inline.hpp from .hpp files
David Holmes
david.holmes at oracle.com
Thu Mar 22 06:06:14 UTC 2018
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.
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::SignatureHandlerGenerator::SignatureHandlerGenerator(
>
>
> +InterpreterRuntime::SignatureHandlerGenerator::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/~coleenp/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/%7Ecoleenp/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.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>>>
>
> I'll update the copyrights when I commit.
>
> Thanks,
> Coleen
>
>
>
>
>
>
More information about the hotspot-dev
mailing list