RFR(L): 8049325: Introduce and clean up umbrella headers for the files in the cpu subdirectories
david.holmes at oracle.com
Wed Jul 9 02:23:48 UTC 2014
On 9/07/2014 3:42 AM, Lois Foltan wrote:
> Hi Goetz,
> Overall this cleanup looks good. Here are specific comments per file:
> - 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.
> - 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)
> - change not related to clean up of umbrella headers, please
> - 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?
> - only has a copyright change in the file, no other changes present?
> - incorrect copyright, no current year?
> - incorrect copyright date for a new file
> - 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?
> On 7/7/2014 4:52 AM, Lindenmaier, Goetz wrote:
>> I decided to clean up the remaining include cascades, too.
>> This change introduces umbrella headers for the files in the cpu
>> 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.
>> I compiled and tested this without precompiled headers on linuxx86_64,
>> 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
>> against jdk9/dev, too.)
>> Best regards,
>> PS: I also did all the Copyright adaptions ;)
More information about the hotspot-dev