RFR: 8352645: Add tool support to check order of includes

[Stefan Karlsson]

On Wed, 26 Mar 2025 09:21:59 GMT, Doug Simon <dnsimon at openjdk.org> wrote:

> This PR adds `test/hotspot/jtreg/sources/SortIncludes.java`, a tool to check that blocks of include statements in C++ files are sorted and that there's at least one blank line between user and sys includes (as per the [style guide](https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md#source-files)).
> 
> By virtue of using `SortedSet`, the tool also removes duplicate includes (e.g. `"compiler/compilerDirectives.hpp"` on line [37](https://github.com/openjdk/jdk/blob/059f190f4b0c7836b89ca2070400529e8d33790b/src/hotspot/share/c1/c1_Compilation.cpp#L37) and line [41](https://github.com/openjdk/jdk/blob/059f190f4b0c7836b89ca2070400529e8d33790b/src/hotspot/share/c1/c1_Compilation.cpp#L41)). Sorting uses lowercased strings so that `_` sorts before letters, preserving the prevailing convention in the code base. I've also updated the style guide to clarify this sort-order.
> 
> The tool does nothing about re-ordering blocks of conditional includes vs unconditional includes. I briefly looked into that but it gets very complicated, very quickly. That kind of re-ordering will have to continue to be done manually for now.
> 
> I have used the tool to fix the ordering of a subset of HotSpot sources and added a test to keep them sorted. That test can be expanded over time to keep includes sorted in other HotSpot directories.
> 
> When `TestIncludesAreSorted.java` fails, it tries to provide actionable advice. For example:
> 
> java.lang.RuntimeException: The unsorted includes listed below should be fixable by running:
> 
>     java /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/sources/SortIncludes.java --update /Users/dnsimon/dev/jdk-jdk/open/src/hotspot/share/c1 /Users/dnsimon/dev/jdk-jdk/open/src/hotspot/share/ci /Users/dnsimon/dev/jdk-jdk/open/src/hotspot/share/compiler /Users/dnsimon/dev/jdk-jdk/open/src/hotspot/share/jvmci
> 
> 	at TestIncludesAreSorted.main(TestIncludesAreSorted.java:80)
> 	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
> 	at java.base/java.lang.reflect.Method.invoke(Method.java:565)
> 	at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:335)
> 	at java.base/java.lang.Thread.run(Thread.java:1447)
> Caused by: java.lang.RuntimeException: 36 files with unsorted headers found:
> 
> /Users/dnsimon/dev/jdk-jdk/open/src/hotspot/share/c1/c1_Compilation.cpp
> /Users/dnsimon/dev/jdk-jdk/open/src/hotspot/share/c1/c1_Runtime1.cpp
> /Users/dnsimon/dev/jdk-jdk/open/src/hotspot/share/c1/c1_Optim...

Thanks for updating to use the lower-case comparison. I wonder if a small tweak can fix the extra blank lines I complained about in the other PR.

The tool removes the extra blank line we have in our .inline.hpp. From the Style Guide:

All .inline.hpp files should include their corresponding .hpp file as the first include line with a blank line separating it from the rest of the include lines. Declarations needed by other files should be put in the .hpp file, and not in the .inline.hpp file. This rule exists to resolve problems with circular dependencies between .inline.hpp files.


I think this needs to be fixed, otherwise people will start to remove these.

src/hotspot/share/compiler/oopMap.inline.hpp line 29:

> 27: 
> 28: #include "compiler/oopMap.hpp"
> 29: 

This blank line should not be removed.

test/hotspot/jtreg/sources/SortIncludes.java line 77:

> 75:             blankLines = List.of("");
> 76:         }
> 77:         result.addAll(blankLines);

If this line is removed you don't get the extra blank lines I mentioned in the previous PR. It also removes the extra blank line that you get inserted into oopMap.inline.hpp before the INCLUDE_JVMCI block.

-------------

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24247#pullrequestreview-2716954567
PR Review Comment: https://git.openjdk.org/jdk/pull/24247#discussion_r2014026694
PR Review Comment: https://git.openjdk.org/jdk/pull/24247#discussion_r2014025793