RFR: 8352645: Add tool support to check order of includes [v2]

[Kim Barrett]

On Wed, 26 Mar 2025 14:23:09 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/dnsimo...
>
> Doug Simon has updated the pull request incrementally with one additional commit since the last revision:
> 
>   drop extra blank lines and preserve rule for first include in .inline.hpp files

Changes requested by kbarrett (Reviewer).

src/hotspot/share/ci/ciUtilities.inline.hpp line 29:

> 27: 
> 28: #include "ci/ciUtilities.hpp"
> 29: 

Extra blank line not removed?

src/hotspot/share/ci/ciUtilities.inline.hpp line 32:

> 30: #include "runtime/interfaceSupport.inline.hpp"
> 31: 
> 32: 

Extra blank line inserted?

src/hotspot/share/compiler/compilationFailureInfo.cpp line 35:

> 33: #include "compiler/compilationFailureInfo.hpp"
> 34: #include "compiler/compileTask.hpp"
> 35: #ifdef COMPILER2

Conditional includes are supposed to follow unconditional in a section.
Out of scope for this PR?

src/hotspot/share/compiler/disassembler.hpp line 36:

> 34: #include "utilities/macros.hpp"
> 35: 
> 36: 

Extra blank line inserted?

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

> 37: 
> 38: public class SortIncludes {
> 39:     private static final String INCLUDE_LINE = "^ *#include *(<[^>]+>|\"[^\"]+\") *$\\n";

There are files that have spaces between the `#` and `include`.  I'm kind of inclined to suggest we fix those
at some point (not in this PR).  But the regex here needs to allow for that possibility, and perhaps (eventually)
complain about such.

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

> 113:     }
> 114: 
> 115:     /// Processes the C++ source file in `path` to sort its include statements.

If we want to apply this to hotspot jtreg test code, then C source files also come into the picture.

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

> 151: 
> 152:     /// Processes the C++ source files in `paths` to check if their include statements are sorted.
> 153:     /// Include statements with any non-space characters after the closing `"` or `>` will not

Perhaps this should be mentioned in the style guide?

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

PR Review: https://git.openjdk.org/jdk/pull/24247#pullrequestreview-2719852021
PR Review Comment: https://git.openjdk.org/jdk/pull/24247#discussion_r2015721384
PR Review Comment: https://git.openjdk.org/jdk/pull/24247#discussion_r2015718606
PR Review Comment: https://git.openjdk.org/jdk/pull/24247#discussion_r2015723999
PR Review Comment: https://git.openjdk.org/jdk/pull/24247#discussion_r2015725371
PR Review Comment: https://git.openjdk.org/jdk/pull/24247#discussion_r2015706803
PR Review Comment: https://git.openjdk.org/jdk/pull/24247#discussion_r2015712545
PR Review Comment: https://git.openjdk.org/jdk/pull/24247#discussion_r2015714360