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

[Stefan Karlsson]

On Thu, 27 Mar 2025 09:49:38 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 four additional commits since the last revision:
> 
>  - allow spaces between `#` and `include`
>  - moved some logic out of SortIncludes into TestIncludesAreSorted
>  - removed extra blank lines
>  - update style guide with advice on how to label includes that should not be re-ordered

I'm happy with the capabilities of the tool now and think that it is good enough to include and promote to HotSpot devs.

One questions is where to put the tool? I don't think the test directory is the best place. Maybe somewhere in `src/utils/`. There is a tools dir here `src/utils/src/build/tools/` but I don't know if it is appropriate to put it there. Maybe @magicus knows a good place for this?

A couple of nits:
1) jcheck fails because of whitespaces
2) The /// style comments is a style I haven't encountered before.

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

PR Review: https://git.openjdk.org/jdk/pull/24247#pullrequestreview-2720671629