On Thu, 20 Jun 2024 08:29:39 GMT, Julian Waters <jwat...@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
