RFR(L): 8049325: Introduce and clean up umbrella headers for the files in the cpu subdirectories

[David Holmes]

On 9/07/2014 10:37 PM, Lois Foltan wrote:
>
> On 7/8/2014 10:23 PM, David Holmes wrote:
>> Hi Lois,
>>
>> On 9/07/2014 3:42 AM, Lois Foltan wrote:
>>> Hi Goetz,
>>>
>>> Overall this cleanup looks good.  Here are specific comments per file:
>>>
>>> src/cpu/ppc/vm/runtime_ppc.cpp
>>>      - include nativeInst.hpp instead of nativeInst_ppc.hpp
>>
>> Hmmm - doesn't this go against the argument Coleen was making with
>> regard to the other umbrella header situation? She said a platform
>> specific file should include the platform specific header rather than
>> the generic top-level header.
>>
>> I must admit I'm not completely convinced as it depends on whether the
>> platform specific implementation calls generic functions that may or
>> may not have a platform specific implementation.
>
> Hi David,
>
> Yes, you are correct, I looked back to some of the email discussion from
> JDK-8048241.  I share your thoughts on this topic but will defer since
> this was a recent discussion.  Goetz, please disregard my comments about
> including <file>.hpp instead of <file>_platform.hpp below.  I think this
> just affected src/cpu/ppc/vm/runtime_ppc.cpp and
> src/cpu/sparc/vm/c1_Runtime1_sparc.cpp.

I'd like to get some clarification on this just to know how to structure 
things. Given:

foo.hpp
foo.inline.hpp
foo.platform.inline.hpp

which of the above should #include which others, and which should be 
#include'd by "client" code?

Thanks,
David

> Thanks,
> Lois
>
>>
>> David
>> -----
>>
>>> src/cpu/sparc/vm/c1_Runtime1_sparc.cpp
>>>      - include nativeInst.hpp instead of nativeInst_sparc.hpp
>>>      - include vmreg.inline.hpp instead of vmreg_sparc.inline.hpp
>>>        (however this could pull in more code than needed since
>>> vmreg.inline.hpp also includes asm/register.hpp and code/vmreg.hpp)
>>>
>>> src/cpu/ppc/vm/stubGenerator_ppc.cpp
>>>      - change not related to clean up of umbrella headers, please
>>> explain/justify.
>>>
>>> src/share/vm/code/vmreg.hpp
>>>      - Can lines #143-#15 be replaced by an inclusion of
>>> vmreg.inline.hpp or will
>>>        this introduce a cyclical inclusion situation, since
>>> vmreg.inline.hpp includes vmreg.hpp?
>>>
>>> src/share/vm/classfile/classFileStream.cpp
>>>      - only has a copyright change in the file, no other changes
>>> present?
>>>
>>> src/share/vm/prims/jvmtiClassFileReconstituter.cpp
>>>      - incorrect copyright, no current year?
>>>
>>> src/share/vm/opto/ad.hpp
>>>      - incorrect copyright date for a new file
>>>
>>> src/share/vm/code/vmreg.inline.hpp
>>>      - technically this new file does not need to include
>>> "asm/register.hpp" since
>>>        vmreg.hpp already includes it
>>>
>>> My only lingering concern is the cyclical nature of
>>> vmreg.hpp/vmreg.inline.hpp.  It might be better to not introduce the new
>>> file "vmreg.inline.hpp" in favor of having files include vmreg.hpp
>>> instead?  Again since vmreg.inline.hpp includes vmreg.hpp there really
>>> is not much difference between the two?
>>>
>>> Thanks,
>>> Lois
>>>
>>>
>>> On 7/7/2014 4:52 AM, Lindenmaier, Goetz wrote:
>>>> Hi,
>>>>
>>>> I decided to clean up the remaining include cascades, too.
>>>>
>>>> This change introduces umbrella headers for the files in the cpu
>>>> subdirectories:
>>>>
>>>> src/share/vm/utilities/bytes.hpp
>>>> src/share/vm/opto/ad.hpp
>>>> src/share/vm/code/nativeInst.hpp
>>>> src/share/vm/code/vmreg.inline.hpp
>>>> src/share/vm/interpreter/interp_masm.hpp
>>>>
>>>> It also cleans up the include cascades for adGlobals*.hpp,
>>>> jniTypes*.hpp, vm_version*.hpp and register*.hpp.
>>>>
>>>> Where possible, this change avoids includes in headers.
>>>> Eventually it adds a forward declaration.
>>>>
>>>> vmreg_<cpu>.inline.hpp contains functions declared in register_cpu.hpp
>>>> and vmreg.hpp, so there is no obvious mapping to the shared files.
>>>> Still, I did not split the files in the cpu directories, as they are
>>>> rather small.
>>>>
>>>> I didn't introduce a file for adGlobals_<cpu>.hpp, as adGlobals mainly
>>>> contains machine dependent, c2 specific register information. So I
>>>> think optoreg.hpp is a good header to place the adGlobals_<cpu>.hpp
>>>> includes in,
>>>> and then use optoreg.hpp where symbols from adGlobals are needed.
>>>>
>>>> I moved the constructor and destructor of CodeletMark to the .cpp
>>>> file, I don't think this is performance relevant. But having them in
>>>> the header requirs to pull interp_masm.hpp into interpreter.hpp, and
>>>> thus all the assembler include headers into a lot of files.
>>>>
>>>> Please review and test this change.  I please need a sponsor.
>>>> http://cr.openjdk.java.net/~goetz/webrevs/8049325-cpuInc/webrev.01/
>>>>
>>>> I compiled and tested this without precompiled headers on linuxx86_64,
>>>> linuxppc64,
>>>> windowsx86_64, solaris_sparc64, solaris_sparc32, darwinx86_64,
>>>> aixppc64, ntamd64
>>>> in opt, dbg and fastdbg versions.
>>>>
>>>> Currently, the change applies to hs-rt, but once my other change
>>>> arrives in other
>>>> repos, it will work there, too.  (I tested it together with the other
>>>> change
>>>> against jdk9/dev, too.)
>>>>
>>>> Best regards,
>>>>    Goetz.
>>>>
>>>> PS: I also did all the Copyright adaptions ;)
>>>
>