On Fri, 11 Nov 2022 14:26:20 GMT, Stefan Karlsson <stef...@openjdk.org> wrote:
> The sorted blocks of includes have deteriorated to the point that I felt
> compelled to clean up some of the issues.
>
> One of the more prevalent issues is that files in src/hotspot/share/include
> are not properly sorted. There has been some discussion that that was done on
> purpose, but it just adds another exception to the include rules that don't
> have any practical purposes, IMHO. It also goes against our written style
> guide around include files. One argument why it was OK have the files in
> include/ pushed up to the top of the sorted block, was that the file was
> included without specifying a directory. That's an argument that contradicts
> how we treat platform-dependent files, which (unfortunately) often also are
> specified without a prefixed directory, so I don't think that's a good enough
> argument, again IMHO. To remove this special case, I've removed the
> extraneous make file entry to have src/hotspot/share/include in the set of
> directories to search for headers when compiling HotSpot. Now all the header
> files in src/hotspot/share/include gets included by specifying the path from
> src/hotspot/share,
just like the other platform-independent headers in HotSpot.
>
> While going over the include headers I've also cleaned up surrounding
> whitespaces and incorrect include guards.
I don't think platform-specific files that are included without providing a
directory should be sorted with the shared files on just the filename. That
just scatters the platform-specific files, in a way that I think isn't
helpful. Such files ought to be at the end, using the appropriate macros.
But there are some filenames that don't have the appropriate stem for use with
those macros. I think those files should be renamed. So I disagree with
moving the includes of these files. I'd prefer they were left as-is by this
change, with a followup to rename files and use the appropriate macros.
There are some files external to hotspot that are being included without any
directory component, such as jni.h and jimage.hpp. Moving them into the middle
of the "shared" includes, sorted by just filename name without a directory,
seems like it is hiding external dependencies. I don't have a well-formed
suggestion for what to do about them, but I think the proposed moves aren't
helpful.
I wonder if standard library includes should also be sorted. I think you
might have done that in some places, but not sure about that. Certainly there
are some that aren't.
There are a number of gtest where a standard library include was moved to
after the include of unittest.hpp. I think unittest.hpp is supposed to always
be the last included header, and added a comment to those places. But now I
can't find anything that says that. (BTW, I agree with moving things like
threadHelper.inline.hpp into a block after normal includes, with
unittest.hpp.)
Related, there are 62 .cpp files missing "precompiled.hpp"
`find . -name "*.cpp" -exec grep -L "precompiled.hpp" {} ;`
At least some (os_linux.cpp, os_posix.cpp) have a comment claiming
intentionality, though that seems suspect (os_linux.cpp has been that way
since before mercurial). I think this should be another followup.
I wish this change had been split up a bit. One change (or a separate commit
within this change) that dealt with files in share/include could have been
mechanically checked or pretty easily verfied by eye. That would have left a
much smaller residue that needed going through more carefully.
Touching a whole mess of gtests to put a blank line between other includes and
the final include of unittest.hpp could also have been separate (PR or commit).
Should there be any additions to the style guide to codify some of these
changes? I can't find anything about where unittest.hpp and other
gtest-specific headers should go. Nor can I find anything about where
standard library headers should go.
src/hotspot/cpu/x86/macroAssembler_x86.cpp line 30:
> 28: #include "compiler/compiler_globals.hpp"
> 29: #include "compiler/disassembler.hpp"
> 30: #include "crc32c.h"
This ought to remain at the end, included using `CPU_HEADER_H("crc32c")`, but
the file doesn't have the appropriate suffix. I think the file name should be
changed so that can be done. This seems to be the only C++ file under cpu/
that doesn't have the appropriate suffix. There are similarly "misnamed" files
under various os/ subdirs; I didn't look for any in os_cpu/.
src/hotspot/os/windows/jvm_windows.cpp line 27:
> 25: #include "precompiled.hpp"
> 26: #include "include/jvm.h"
> 27: #include "os_windows.hpp"
os_windows should be at the end, included using `OS_HEADER("os")`.
src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp line 57:
> 55: #if !defined(__APPLE__) && !defined(__NetBSD__)
> 56: #include <pthread.h>
> 57: # include <pthread_np.h> /* For pthread_attr_get_np */
Remove unneeded space.
src/hotspot/share/cds/classListParser.cpp line 42:
> 40: #include "interpreter/bytecodeStream.hpp"
> 41: #include "interpreter/linkResolver.hpp"
> 42: #include "jimage.hpp"
Sorting jimage.hpp with hotspot/share stuff seems weird, and is kind of hiding
this external dependency. (It's coming from java.base.)
src/hotspot/share/prims/scopedMemoryAccess.cpp line 34:
> 32: #include "runtime/interfaceSupport.inline.hpp"
> 33: #include "runtime/jniHandles.inline.hpp"
> 34: #include "runtime/deoptimization.hpp"
runtime/deoptimization.hpp should be at the front of the runtime/ list.
src/hotspot/share/prims/whitebox.cpp line 26:
> 24:
> 25: #include "precompiled.hpp"
> 26: #include <new>
Why was `<new>` removed?
test/hotspot/gtest/jfr/test_adaptiveSampler.cpp line 48:
> 46: #include "unittest.hpp"
> 47:
> 48: #include <cmath>
Why is this after unittest.hpp?
test/hotspot/gtest/metaprogramming/test_enableIf.cpp line 32:
> 30: #include "unittest.hpp"
> 31:
> 32: #include <type_traits>
Why is this following unittest.hpp.
test/hotspot/gtest/runtime/test_os_linux.cpp line 28:
> 26: #ifdef LINUX
> 27:
> 28: #include "os_linux.hpp"
Why do we even need this - we have runtime/os.hpp and we're on Linux.
test/hotspot/gtest/utilities/test_align.cpp line 31:
> 29: #include "unittest.hpp"
> 30:
> 31: #include <limits>
Why after unittest.hpp?
test/hotspot/gtest/utilities/test_bitMap_setops.cpp line 34:
> 32: #include "unittest.hpp"
> 33:
> 34: #include <stdlib.h>
Why after unittest.hpp.
test/hotspot/gtest/utilities/test_count_leading_zeros.cpp line 32:
> 30: #include "unittest.hpp"
> 31:
> 32: #include <limits>
[pre-existing] Why after unittest.hpp?
test/hotspot/gtest/utilities/test_enumIterator.cpp line 29:
> 27: #include "unittest.hpp"
> 28:
> 29: #include <type_traits>
Why after unittest.hpp?
test/hotspot/gtest/utilities/test_globalDefinitions.cpp line 33:
> 31: #include "unittest.hpp"
> 32:
> 33: #include <type_traits>
Why after unittest.hpp.
test/hotspot/gtest/utilities/test_nonblockingQueue.cpp line 32:
> 30: #include "threadHelper.inline.hpp"
> 31: #include "unittest.hpp"
> 32: #include <new>
Why removing `<new>`? (and pre-existing, why after unittest.hpp?)
test/hotspot/gtest/utilities/test_population_count.cpp line 33:
> 31: #include "unittest.hpp"
> 32:
> 33: #include <limits>
Why after unittest.hpp?
test/hotspot/gtest/utilities/test_powerOfTwo.cpp line 31:
> 29: #include "unittest.hpp"
> 30:
> 31: #include <limits>
Why after unittest.hpp?
-------------
Changes requested by kbarrett (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11108
