RFR: 8334599: Improve code from JDK-8302671

Alexey Ivanov aivanov at openjdk.org
Mon Jun 24 21:10:10 UTC 2024


On Thu, 20 Jun 2024 08:29:39 GMT, Julian Waters <jwaters at openjdk.org> wrote:

> In [JDK-8302671](https://bugs.openjdk.org/browse/JDK-8302671) I fixed a memmove decay bug by rewriting a sizeof on an array to an explicit size of 256, but this is a bit of a band aid fix. It's come to my attention that in C++, one can pass an array by reference, which causes sizeof to work correctly on an array and has the added bonus of enforcing an array of that size on the arguments passed to that method. I've reverted my change from 8302671 and instead explicitly made kstate an array reference so that sizeof works on the array as expected, and that the array size can be explicitly set in the array brackets
> 
> Verification: https://godbolt.org/z/Ezj76eWWY and GitHub Actions

Looks good to me.

I added a suggestion to use the `enum` constant declared in `AwtToolkit` instead of hard-coding the size of the array.

src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3366:

> 3364: static void
> 3365: resetKbdState( BYTE (&kstate)[256]) {
> 3366:     BYTE tmpState[256];

Suggestion:

resetKbdState( BYTE (&kstate)[AwtToolkit::KB_STATE_SIZE]) {
    BYTE tmpState[AwtToolkit::KB_STATE_SIZE];

Will this resolve Phil's concern? Both arrays will use the same size.

src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3368:

> 3366:     BYTE tmpState[256];
> 3367:     WCHAR wc[2];
> 3368:     memmove(tmpState, kstate, sizeof(kstate));

Using `memcpy` could be more performant, we know for sure that `tmpState` and `kstate` do not overlap.

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

Marked as reviewed by aivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19798#pullrequestreview-2136706441
PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1651586867
PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1651589012


More information about the client-libs-dev mailing list