On Sun, 23 Mar 2025 21:14:47 GMT, Doug Simon <dnsi...@openjdk.org> wrote:
> This PR adds `bin/sort_includes.py`, a python3 script to check that blocks of > include statements in C++ files are sorted alphabetically 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)). > This script can also update files with unsorted includes. The second commit > in this PR shows the result of running: > > python3 ./bin/sort_includes.py ./src/hotspot > > To prevent an include being reordered, put at least one non-space character > after the closing `"` or `>`. See `src/hotspot/share/adlc/archDesc.cpp` for > an example. > > Assuming this PR is integrated, jcheck could be updated to use it to ensure > include statements remain sorted. Thanks for taking the time to write this tool! I haven't reviewed the script, but I looked over the resulting changes to the includes and have commented on places where I see a discrepancy between what the tool generated and what I would have changed the code to be. Some of my comments are things that we would be great if the tool could fix, but I also think that we could be pragmatic and just fix these manually. OTOH, if we need to do a bunch of manual adjustment then it might not be ready to be included in jcheck. I think it depends on how this tool is going to be used. If it is going to be an authoritative tool for our includes, then it probably needs to handle some of the cases that I listed. With that said, even if there are some back-and-forth discussion about the tool, I think pushing the sort-order fixes by themselves would be worthwhile in it self. Thanks again! src/hotspot/cpu/aarch64/foreignGlobals_aarch64.cpp line 32: > 30: #include "prims/vmstorage.hpp" > 31: #include "runtime/jniHandles.hpp" > 32: #include "runtime/jniHandles.inline.hpp" I know that you didn't introduce this, but I wanted to point out that the convention we are using is to not include the .hpp file if the associated .inline.hpp is included. src/hotspot/cpu/arm/gc/g1/g1BarrierSetAssembler_arm.cpp line 32: > 30: #include "gc/g1/g1HeapRegion.hpp" > 31: #include "gc/g1/g1ThreadLocalData.hpp" > 32: #include "gc/g1/g1ThreadLocalData.hpp" Not your tools fault, but in case you also want to handle this case, we have two includes of the same file here. src/hotspot/cpu/ppc/vm_version_ppc.cpp line 44: > 42: #if defined(_AIX) > 43: #include "os_aix.hpp" > 44: For this file I would have expected the separation of system includes be done as follows: #include "utilities/powerOfTwo.hpp" #if defined(_AIX) #include "os_aix.hpp" #endif #include <sys/sysinfo.h> #if defined(_AIX) #include <libperfstat.h> #endif src/hotspot/os/aix/os_aix.cpp line 98: > 96: #include <sys/ipc.h> > 97: #include <sys/mman.h> > 98: #include <unistd.h> FWIW, it would be interesting to hear if the AIX maintainers really need the define in the middle of the system includes ... src/hotspot/os/aix/porting_aix.cpp line 30: > 28: #define __XCOFF64__ > 29: #include <xcoff.h> > 30: This blank line should probably be left in place. (It would also be nice to have a blankline between line 23 and 24, but not your tool's job to fix that) src/hotspot/os/bsd/memMapPrinter_macosx.cpp line 34: > 32: #include "utilities/powerOfTwo.hpp" > 33: > 34: One too many blanklines. src/hotspot/os/linux/osContainer_linux.cpp line 34: > 32: #include <errno.h> > 33: #include <math.h> > 34: #include <string.h> There are too blank lines after the includes. (In case you want to enhance your tool to also clean that out) src/hotspot/os/posix/safefetch_sigjmp.cpp line 36: > 34: // For SafeFetch we need POSIX TLS and sigsetjmp/longjmp. > 35: #include <pthread.h> > 36: #include <setjmp.h> Missing blankline after this include. (In case you want to add support for this cleanup in your tool) src/hotspot/os/windows/os_windows.cpp line 108: > 106: #include <imagehlp.h> // For os::dll_address_to_function_name > 107: // for enumerating dll libraries > 108: #include <vdmdbg.h> The include moved, but the comment was left, so the comment now refers to the wrong include (I think). FWIW, I tend to remove these comments about why they are included because they become obsolete if you start to use more things from the headers. src/hotspot/os/windows/symbolengine.cpp line 31: > 29: #include "windbghelp.hpp" > 30: > 31: #include <windows.h> This left two consecutive blanklines. src/hotspot/os/windows/systemMemoryBarrier_windows.cpp line 27: > 25: #include "systemMemoryBarrier_windows.hpp" > 26: > 27: #include <windows.h> I'm a little curious about why this was needed, but just a little bit ... src/hotspot/share/adlc/archDesc.cpp line 27: > 25: > 26: // archDesc.cpp - Internal format for architecture definition > 27: #include <unordered_set> // do not reorder I *guess* that this has to do with the assert define. I think Kim has another workaround for that. src/hotspot/share/cds/archiveBuilder.cpp line 51: > 49: #include "memory/allStatic.hpp" > 50: #include "memory/memRegion.hpp" > 51: #include "memory/memoryReserver.hpp" I think this can be argued is a bug. In previous discussion we (or, at least I) have argued that the sort order should be case-insensitive. src/hotspot/share/compiler/compilationFailureInfo.cpp line 37: > 35: #ifdef COMPILER2 > 36: #include "opto/compile.hpp" > 37: #include "opto/node.hpp" Here the entire: #ifdef COMPILER2 #include "opto/compile.hpp" #include "opto/node.hpp" #endif block should be last. I don't know if your tool should try to fix this. src/hotspot/share/gc/g1/g1ConcurrentRebuildAndScrub.cpp line 40: > 38: #include "utilities/globalDefinitions.hpp" > 39: > 40: Stray extra blankline src/hotspot/share/gc/shared/gcConfiguration.cpp line 27: > 25: #include "gc/shared/gcArguments.hpp" > 26: #include "gc/shared/gcConfiguration.hpp" > 27: #include "gc/shared/gc_globals.hpp" I think the manual sorting order placed _ before letter. Your tool undos that. I think this is OK, but I wanted to point this out so that this is done intentionally. src/hotspot/share/gc/shenandoah/heuristics/shenandoahGenerationalHeuristics.cpp line 37: > 35: #include "logging/log.hpp" > 36: > 37: Stray extra blankline src/hotspot/share/gc/shenandoah/heuristics/shenandoahGlobalHeuristics.cpp line 33: > 31: #include "utilities/quickSort.hpp" > 32: > 33: Stray extra blankline src/hotspot/share/gc/shenandoah/heuristics/shenandoahYoungHeuristics.cpp line 35: > 33: #include "utilities/quickSort.hpp" > 34: > 35: Stray extra blankline src/hotspot/share/gc/shenandoah/shenandoahController.cpp line 30: > 28: #include "gc/shenandoah/shenandoahHeap.hpp" > 29: #include "gc/shenandoah/shenandoahHeapRegion.inline.hpp" > 30: #include "shenandoahCollectorPolicy.hpp" (I think it would be nice if Shenandoah maintainers added the gc/shenandoah/ to this include) src/hotspot/share/gc/shenandoah/shenandoahController.cpp line 31: > 29: #include "gc/shenandoah/shenandoahHeapRegion.inline.hpp" > 30: #include "shenandoahCollectorPolicy.hpp" > 31: Stray extra blankline src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 43: > 41: #include "utilities/quickSort.hpp" > 42: > 43: Stray extra blankline (there are probably more, but I'll skip this) src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 81: > 79: #include "gc/shenandoah/shenandoahYoungGeneration.hpp" > 80: > 81: This section has three blanklines, but there should be none if this is to follow the rest of the HotSpot include style. src/hotspot/share/opto/output.cpp line 39: > 37: #include "opto/block.hpp" > 38: #include "opto/c2_MacroAssembler.hpp" > 39: #include "opto/c2compiler.hpp" Hmm. #include "opto/c2_MacroAssembler.hpp" #include "opto/c2compiler.hpp" Here _ comes before lower-case letters. From other places I see that _ comes after upper-case letters. I realize that this is caused by the ASCII value, but it is a bit unfortunate (IMHO). OTOH, maybe this will be consistent (the way I would like it, at least) if the sorting was done on lower-cased strings. src/hotspot/share/prims/foreignGlobals.cpp line 25: > 23: > 24: #include "classfile/javaClasses.hpp" > 25: #include "foreignGlobals.hpp" (For maintainers of this file, this should be prefixed with prims/) src/hotspot/share/services/diagnosticCommand.cpp line 72: > 70: #ifdef LINUX > 71: #include "mallocInfoDcmd.hpp" > 72: #include "os_posix.hpp" This should probably be: #ifdef LINUX #include "mallocInfoDcmd.hpp" #include "os_posix.hpp" #include "trimCHeapDCmd.hpp" #endif #ifdef LINUX #include <errno.h> #endif (and missing prefix should be added by maintainers) ------------- Changes requested by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24180#pullrequestreview-2714925291 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012784742 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012808108 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012812085 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012816294 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012818838 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012819309 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012822001 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012823415 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012826026 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012826486 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012829497 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012853306 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012856105 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012861794 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012862974 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012866552 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012867561 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012867729 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012867935 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012869598 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012868391 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012871019 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012872217 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012885535 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012877987 PR Review Comment: https://git.openjdk.org/jdk/pull/24180#discussion_r2012889539
