[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8296886: Fix various include sort order issues [v2]
From:       Stefan Karlsson <stefank () openjdk ! org>
Date:       2022-11-24 13:21:33
Message-ID: aB2D2T5oao8JflrSY4Z7sVOqRW9XaDbbwBXWaAvgxko=.5037b486-5396-4f8e-abfc-6bce36d9c5a1 () github ! com
[Download RAW message or body]

On Wed, 16 Nov 2022 16:17:48 GMT, Stefan Karlsson <stefank@openjdk.org> wrote:

> > The sorted blocks of includes have deteriorated to the point that I felt \
> > compelled to clean up some of the issues. 
> > *EDIT*: The below discussion has been deferred out of this PR. Now this only \
> > deals with fixing the placement and sorting of includes, plus some surrounding \
> > blank lines. 
> > One of the more prevalent issues is that files in src/hotspot/share/include are \
> > not properly sorted. There has been some discussion that that was done on \
> > purpose, but it just adds another exception to the include rules that don't have \
> > any practical purposes, IMHO. It also goes against our written style guide around \
> > include files. One argument why it was OK have the files in include/ pushed up to \
> > the top of the sorted block, was that the file was included without specifying a \
> > directory. That's an argument that contradicts how we treat platform-dependent \
> > files, which (unfortunately) often also are specified without a prefixed \
> > directory, so I don't think that's a good enough argument, again IMHO. To remove \
> > this special case, I've removed the extraneous make file entry to have \
> > src/hotspot/share/include in the set of directories to search for headers when \
> > compiling HotSpot. Now all the header files in src/hotspot/share/include gets \
> > included by specifying the path from src/hotspot/share
 , just like the other platform-independent headers in HotSpot.
> > 
> > While going over the include headers I've also cleaned up surrounding whitespaces \
> > and incorrect include guards.
> 
> Stefan Karlsson has updated the pull request with a new target base due to a merge \
> or a rebase. The pull request now contains three commits: 
> - Cleanups
> - Merge remote-tracking branch 'upstream/master' into various_include_order_fixes
> - Various include order fixes

Thanks all for reviewing! I'm going to let the GHA run before integrating this \
change.

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

PR: https://git.openjdk.org/jdk/pull/11108


[prev in list] [next in list] [prev in thread] [next in thread]