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

Lois Foltan lois.foltan at oracle.com
Tue Jul 8 17:42:12 UTC 2014


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

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 ;)



More information about the hotspot-dev mailing list