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

[Doug Simon]

On Thu, 27 Mar 2025 06:26:43 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

> Probably we want to eventually apply this to gtests, but there might be additional rules there. The include of unittest.hpp is (usually) last, and there may be (or may have been) a technical reason for that.
> 
> Applying it to jtreg test support files could also introduce some challenges. Or at least discover a lot of non-conforming files. We might eventually want a mechanism for excluding directories, in addition to an inclusion list (that might eventually be "all").
> 
> These kinds of things can be followups once we have the basic mechanism in place.

I would suggest someone open issue(s) for follow up enhancements to the tool. I think having something in place now and incrementally improving it and adjusting it for all the special cases makes most sense.

> 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?

Yep. From the PR description:

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.

> 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.

I think the tool will need to be updated to handle C source files. At that point, the comment should be generalized.

> 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?

Done.

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

PR Comment: https://git.openjdk.org/jdk/pull/24247#issuecomment-2757350491
PR Review Comment: https://git.openjdk.org/jdk/pull/24247#discussion_r2016078724
PR Review Comment: https://git.openjdk.org/jdk/pull/24247#discussion_r2016077938
PR Review Comment: https://git.openjdk.org/jdk/pull/24247#discussion_r2016078194