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&pr=11682&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11682&range=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
Re: RFR: 8298241: Replace C-style casts with JavaThread::cast
On Wed, 14 Dec 2022 22:22:48 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. Looks good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.org/jdk/pull/11682
Re: RFR: 8298241: Replace C-style casts with JavaThread::cast
On Wed, 14 Dec 2022 22:22:48 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.
Marked as reviewed by stefank (Reviewer).
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.
-
PR: https://git.openjdk.org/jdk/pull/11682
Re: RFR: 8298241: Replace C-style casts with JavaThread::cast
On Wed, 14 Dec 2022 23:13:30 GMT, Coleen Phillimore 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. > > Looks good! Thanks @coleenp ! - PR: https://git.openjdk.org/jdk/pull/11682
Re: RFR: 8298241: Replace C-style casts with JavaThread::cast
On Wed, 14 Dec 2022 22:22:48 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 (TBD) > - Sanity testing tiers 1-3 > Thanks. Looks good! - Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.org/jdk/pull/11682
RFR: 8298241: Replace C-style casts with JavaThread::cast
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 (TBD) - Sanity testing tiers 1-3 Thanks. - Commit messages: - 8298241: Replace C-style casts with JavaThread::cast Changes: https://git.openjdk.org/jdk/pull/11682/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11682&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8298241 Stats: 44 lines in 19 files changed: 0 ins; 11 del; 33 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
