Re: RFR: 8298241: Replace C-style casts with JavaThread::cast [v2]
On Thu, 15 Dec 2022 21:20:31 GMT, David Holmes wrote: >> This is a simple cleanup RFE to get rid of old-style C casts in relation to >> JavaThread. >> >> In many cases involving NULL/nullptr the cast could just be dropped. >> Sometimes a static cast is needed to disambiguate overloads. >> >> A couple of reinterpret_cast are needed when doing int/ptr conversion. >> >> static_cast is used for void* conversion. >> >> The other changes should be self explanatory. >> >> The changes in >> >> src/hotspot/os_cpu/bsd_aarch64/javaThread_bsd_aarch64.cpp >> src/hotspot/os_cpu/windows_aarch64/javaThread_windows_aarch64.cpp >> >> are a bit more extensive. That code was using an alias for `this` which is >> completely unnecessary (and the alias creation was using the cast). This >> could be factored out if thought necessary. >> >> I grep'd as best I could for the old C-style casts but can't be certain I >> got them all. >> >> Testing: >> - all builds in our tiers1-5 >> - local linux x64 product, slowdebug and fastdebug >> - GHA >> - Sanity testing tiers 1-3 >> Thanks. > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed commented examle. Ok, thanks. That's a shame. It seems like being able to run style checkers and other linters would be useful in cases like this. - PR: https://git.openjdk.org/jdk/pull/11682
Re: RFR: 8298241: Replace C-style casts with JavaThread::cast [v2]
On Thu, 15 Dec 2022 22:21:35 GMT, Doug Simon wrote: >> David Holmes has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed commented examle. > > Is it possible to add a test based on grep to prevent re-introduction of the > unwanted C-style casts? Sorry @dougxc only found your comment by accident. AFAIK we don't have any such style of tests that would try to check the source code for an anti-pattern. Our test suites don't have access to the src tree when they run. - PR: https://git.openjdk.org/jdk/pull/11682
Re: RFR: 8298241: Replace C-style casts with JavaThread::cast [v2]
On Thu, 15 Dec 2022 21:20:31 GMT, David Holmes wrote: >> This is a simple cleanup RFE to get rid of old-style C casts in relation to >> JavaThread. >> >> In many cases involving NULL/nullptr the cast could just be dropped. >> Sometimes a static cast is needed to disambiguate overloads. >> >> A couple of reinterpret_cast are needed when doing int/ptr conversion. >> >> static_cast is used for void* conversion. >> >> The other changes should be self explanatory. >> >> The changes in >> >> src/hotspot/os_cpu/bsd_aarch64/javaThread_bsd_aarch64.cpp >> src/hotspot/os_cpu/windows_aarch64/javaThread_windows_aarch64.cpp >> >> are a bit more extensive. That code was using an alias for `this` which is >> completely unnecessary (and the alias creation was using the cast). This >> could be factored out if thought necessary. >> >> I grep'd as best I could for the old C-style casts but can't be certain I >> got them all. >> >> Testing: >> - all builds in our tiers1-5 >> - local linux x64 product, slowdebug and fastdebug >> - GHA >> - Sanity testing tiers 1-3 >> Thanks. > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed commented examle. Is it possible to add a test based on grep to prevent re-introduction of the unwanted C-style casts? - PR: https://git.openjdk.org/jdk/pull/11682
Re: RFR: 8298241: Replace C-style casts with JavaThread::cast [v2]
On Thu, 15 Dec 2022 07:07:26 GMT, Stefan Karlsson wrote:
>> David Holmes has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed commented examle.
>
> Marked as reviewed by stefank (Reviewer).
Thanks for the reviews @stefank and @sspitsyn !
> src/hotspot/share/runtime/frame.cpp line 1568:
>
>> 1566: }
>> 1567: }
>> 1568: // if (error) { tty->cr(); print_on(nullptr, tty); }
>
> This probably need the same cast as in print_frame_layout, to disambiguate
> the call.
Fixed
-
PR: https://git.openjdk.org/jdk/pull/11682
Re: RFR: 8298241: Replace C-style casts with JavaThread::cast [v2]
> This is a simple cleanup RFE to get rid of old-style C casts in relation to > JavaThread. > > In many cases involving NULL/nullptr the cast could just be dropped. > Sometimes a static cast is needed to disambiguate overloads. > > A couple of reinterpret_cast are needed when doing int/ptr conversion. > > static_cast is used for void* conversion. > > The other changes should be self explanatory. > > The changes in > > src/hotspot/os_cpu/bsd_aarch64/javaThread_bsd_aarch64.cpp > src/hotspot/os_cpu/windows_aarch64/javaThread_windows_aarch64.cpp > > are a bit more extensive. That code was using an alias for `this` which is > completely unnecessary (and the alias creation was using the cast). This > could be factored out if thought necessary. > > I grep'd as best I could for the old C-style casts but can't be certain I got > them all. > > Testing: > - all builds in our tiers1-5 > - local linux x64 product, slowdebug and fastdebug > - GHA > - Sanity testing tiers 1-3 > Thanks. David Holmes has updated the pull request incrementally with one additional commit since the last revision: Fixed commented examle. - Changes: - all: https://git.openjdk.org/jdk/pull/11682/files - new: https://git.openjdk.org/jdk/pull/11682/files/5641507a..34f2745d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11682=01 - incr: https://webrevs.openjdk.org/?repo=jdk=11682=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11682.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11682/head:pull/11682 PR: https://git.openjdk.org/jdk/pull/11682
