RFR: 8314694: Separate checked_cast from globalDefinitions.hpp [v2]
Serguei Spitsyn
sspitsyn at openjdk.org
Wed Aug 23 09:09:20 UTC 2023
On Tue, 22 Aug 2023 21:25:48 GMT, Kim Barrett <kbarrett at 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
>
> Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 16 additional commits since the last revision:
>
> - add include needed post-JDK-8314274
> - Merge branch 'master' into move-checked-cast
> - include checkedCast.hpp in cpu files
> - include checkedCast.hpp in cpu/aarch64 files
> - include checkedCast.hpp in cpu/x86 files
> - include checkedCast.hpp in os files
> - include checkedCast.hpp in remaining share files
> - include checkedCast.hpp in classfile files
> - include checkedCast.hpp in code files
> - include checkedCast.hpp in oops files
> - ... and 6 more: https://git.openjdk.org/jdk/compare/f0750585...3a3fbb0b
Changes in `src/hotspot/share/prims` folder look good.
Thanks,
Serguei
-------------
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15377#pullrequestreview-1590815334
More information about the serviceability-dev
mailing list