On Tue, 22 Aug 2023 06:54:20 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> Please review this change which moves checked_cast from globalDefinitions.hpp >> to a separate file. As part of this change we modify files that use >> checked_cast to directly include that new file. There are around 80 such >> files, and that change constitutes the majority of the changed files and >> lines >> in this PR. >> >> This PR doesn't fix the definition of checked_cast (see JDK-8314258). It >> just >> moves the existing definition to a new file, in preparation for fixing it >> later. (I'm running tests on a fixed implementation.) >> >> An alternative is to move checked_cast to a new file but have >> globalDefinitions.hpp include that new file. This would avoid touching the >> include lists of currently using files. It seems to me better to actually >> separate it. >> >> Fortunately, there was only one copyright update needed. Most of the uses >> were >> added recently as part of addressing -Wconversion warnings, so those files >> have already had copyright updates recently. >> >> The other change was to move pointer_delta_as_int next to the related >> pointer_delta, and change it to use a direct assert and static_cast, rather >> than checked_cast. >> >> With the exception of the simple change to pointer_delta_as_int the changes >> in this PR are very simple and almost mechanical. To find the files needing >> an additional include and to demonstrate completing that task, I applied this >> command to the hotspot directory: >> >> >> egrep -r --files-with-matches --exclude-dir=.git " checked_cast<" . | \ >> xargs egrep --files-without-match "utilities/checkedCast.hpp" >> >> >> So perhaps this change is trivial, despite the number of files. >> >> Testing: >> mach5 tier1 > > Looks good. > > Interesting how many of the touched cpp files did not include > globalDefinitions.hpp. Thanks for reviews @tstuefe and @coleenp . > Interesting how many of the touched cpp files did not include > globalDefinitions.hpp. Oh, there are much worse things than that! Take a look at utilities/elfFile.hpp :( There was some exploration of the "Include What You Use" tool a long time ago, but it seemed like it didn't quite do what we wanted. https://include-what-you-use.org/ I'll run the command to check for using files that are missing the new include before integration. That might show more files needing the include added. For example, https://github.com/openjdk/jdk/pull/15289 is ready to integrate. ------------- PR Comment: https://git.openjdk.org/jdk/pull/15377#issuecomment-1688526986
