Re: RFR: 8334599: Improve code from JDK-8302671 [v3]
On Thu, 11 Jul 2024 08:41:28 GMT, Julian Waters 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 > > Julian Waters has updated the pull request incrementally with two additional > commits since the last revision: > > - Revert Formatting in awt_Component.cpp > - Update src/java.desktop/windows/native/libawt/windows/awt_Component.cpp > >Co-authored-by: Alexey Ivanov I guess Phil forgot about this. Seems like it's good to go, though - PR Comment: https://git.openjdk.org/jdk/pull/19798#issuecomment-2268187102
Re: RFR: 8334599: Improve code from JDK-8302671 [v2]
On Thu, 11 Jul 2024 08:36:23 GMT, Julian Waters wrote:
>> src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3365:
>>
>>> 3363: }
>>> 3364: static void
>>> 3365: resetKbdState(BYTE () [AwtToolkit::KB_STATE_SIZE]) {
>>
>> I don't know what is most used syntax for this type. I'd rather keep them
>> together without a space between `()` and `[]`.
>
> Will do
Here, I'd remove the space after the opening parenthesis: `resetKbdState(BYTE`.
It's not worth changing at this point, though…
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1687016681
Re: RFR: 8334599: Improve code from JDK-8302671 [v2]
On Thu, 11 Jul 2024 08:36:54 GMT, Julian Waters wrote: >> src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3368: >> >>> 3366: BYTE tmpState[AwtToolkit::KB_STATE_SIZE]; >>> 3367: WCHAR wc[2]; >>> 3368: memmove(tmpState, kstate, sizeof (kstate)); >> >> Suggestion: >> >> memmove(tmpState, kstate, sizeof(kstate)); >> >> There's usually no space after `sizeof`. > > Ah, seems awt follows a different convention from HotSpot, which I'm nmore > used to. I'll revert the formatting I just searched for `sizeof` in the file. The next function has memset(kbdState, 0, sizeof (kbdState)); with a space. Other usages in the file don't. I'm following the convention used in the file. Please, don't change this single instance above, let it stay. - PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1686995038
Re: RFR: 8334599: Improve code from JDK-8302671 [v3]
On Thu, 11 Jul 2024 08:41:28 GMT, Julian Waters 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 > > Julian Waters has updated the pull request incrementally with two additional > commits since the last revision: > > - Revert Formatting in awt_Component.cpp > - Update src/java.desktop/windows/native/libawt/windows/awt_Component.cpp > >Co-authored-by: Alexey Ivanov Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19798#pullrequestreview-2192300208
Re: RFR: 8334599: Improve code from JDK-8302671 [v3]
On Wed, 17 Jul 2024 17:28:21 GMT, Thomas Stuefe wrote: > Okay. > > (Please don't go now and change all occurrences of `foo(pointer, size)` to > something like `foo( [&] [size] )` :-) I dislike the increased cognitive load > of this form, and prefer the classic pointer+size combo ) Even if I wanted to, that wouldn't be possible, since this syntax is only allowed when passing in an array of known size directly to the method, and the array has to match the parameter declaration in the method exactly ;) Will wait for approval from Phil and Alexey before pushing - PR Comment: https://git.openjdk.org/jdk/pull/19798#issuecomment-2235323641
Re: RFR: 8334599: Improve code from JDK-8302671 [v3]
On Thu, 11 Jul 2024 08:41:28 GMT, Julian Waters 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 > > Julian Waters has updated the pull request incrementally with two additional > commits since the last revision: > > - Revert Formatting in awt_Component.cpp > - Update src/java.desktop/windows/native/libawt/windows/awt_Component.cpp > >Co-authored-by: Alexey Ivanov Okay. (Please don't go now and change all occurrences of `foo(pointer, size)` to something like `foo( [&] [size] )` :-) I dislike the increased cognitive load of this form, and prefer the classic pointer+size combo ) - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19798#pullrequestreview-2183561466
Re: RFR: 8334599: Improve code from JDK-8302671 [v3]
On Thu, 11 Jul 2024 08:41:28 GMT, Julian Waters 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 > > Julian Waters has updated the pull request incrementally with two additional > commits since the last revision: > > - Revert Formatting in awt_Component.cpp > - Update src/java.desktop/windows/native/libawt/windows/awt_Component.cpp > >Co-authored-by: Alexey Ivanov Sorry, am awaiting re-review from all of you - PR Comment: https://git.openjdk.org/jdk/pull/19798#issuecomment-2227730102
Re: RFR: 8334599: Improve code from JDK-8302671 [v2]
On Thu, 4 Jul 2024 12:59:48 GMT, Alexey Ivanov wrote:
>> Julian Waters 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 five additional
>> commits since the last revision:
>>
>> - Formatting sizeof awt_Component.cpp
>> - Formatting awt_Component.cpp
>> - Merge branch 'openjdk:master' into patch-10
>> - Update src/java.desktop/windows/native/libawt/windows/awt_Component.cpp
>>
>>Co-authored-by: Alexey Ivanov
>> - 8334599
>
> src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3365:
>
>> 3363: }
>> 3364: static void
>> 3365: resetKbdState(BYTE () [AwtToolkit::KB_STATE_SIZE]) {
>
> I don't know what is most used syntax for this type. I'd rather keep them
> together without a space between `()` and `[]`.
Will do
> src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3368:
>
>> 3366: BYTE tmpState[AwtToolkit::KB_STATE_SIZE];
>> 3367: WCHAR wc[2];
>> 3368: memmove(tmpState, kstate, sizeof (kstate));
>
> Suggestion:
>
> memmove(tmpState, kstate, sizeof(kstate));
>
> There's usually no space after `sizeof`.
Ah, seems awt follows a different convention from HotSpot, which I'm nmore used
to. I'll revert the formatting
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1673629959
PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1673630629
Re: RFR: 8334599: Improve code from JDK-8302671 [v3]
> 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 Julian Waters has updated the pull request incrementally with two additional commits since the last revision: - Revert Formatting in awt_Component.cpp - Update src/java.desktop/windows/native/libawt/windows/awt_Component.cpp Co-authored-by: Alexey Ivanov - Changes: - all: https://git.openjdk.org/jdk/pull/19798/files - new: https://git.openjdk.org/jdk/pull/19798/files/b98068e9..d61d2503 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19798=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19798=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19798.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19798/head:pull/19798 PR: https://git.openjdk.org/jdk/pull/19798
Re: RFR: 8334599: Improve code from JDK-8302671 [v2]
On Wed, 3 Jul 2024 13:34:38 GMT, Julian Waters 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 > > Julian Waters 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 five additional > commits since the last revision: > > - Formatting sizeof awt_Component.cpp > - Formatting awt_Component.cpp > - Merge branch 'openjdk:master' into patch-10 > - Update src/java.desktop/windows/native/libawt/windows/awt_Component.cpp > >Co-authored-by: Alexey Ivanov > - 8334599 I'd leave the memmove as it is unless someone can show a measurable benefit to changing to memcpy. I do agree with the suggested formatting changes. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19798#pullrequestreview-2167213082
Re: RFR: 8334599: Improve code from JDK-8302671 [v2]
On Thu, 4 Jul 2024 13:15:28 GMT, Thomas Stuefe wrote: > Using memmove is so uncommon that it is usually a clear indication for a > deliberate choice. It may still be an accidental choice. I didn't find any code review for [JDK-6680988](https://bugs.openjdk.org/browse/JDK-6680988) where this line had been added. - PR Comment: https://git.openjdk.org/jdk/pull/19798#issuecomment-2209220729
Re: RFR: 8334599: Improve code from JDK-8302671 [v2]
On Wed, 3 Jul 2024 13:34:38 GMT, Julian Waters 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
>
> Julian Waters 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 five additional
> commits since the last revision:
>
> - Formatting sizeof awt_Component.cpp
> - Formatting awt_Component.cpp
> - Merge branch 'openjdk:master' into patch-10
> - Update src/java.desktop/windows/native/libawt/windows/awt_Component.cpp
>
>Co-authored-by: Alexey Ivanov
> - 8334599
Still looks good, except for the minor formatting comments.
src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3365:
> 3363: }
> 3364: static void
> 3365: resetKbdState(BYTE () [AwtToolkit::KB_STATE_SIZE]) {
I don't know what is most used syntax for this type. I'd rather keep them
together without a space between `()` and `[]`.
src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3368:
> 3366: BYTE tmpState[AwtToolkit::KB_STATE_SIZE];
> 3367: WCHAR wc[2];
> 3368: memmove(tmpState, kstate, sizeof (kstate));
Suggestion:
memmove(tmpState, kstate, sizeof(kstate));
There's usually no space after `sizeof`.
-
Marked as reviewed by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19798#pullrequestreview-2158844075
PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1665680146
PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1665657251
Re: RFR: 8334599: Improve code from JDK-8302671 [v2]
On Wed, 3 Jul 2024 13:34:38 GMT, Julian Waters 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 > > Julian Waters 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 five additional > commits since the last revision: > > - Formatting sizeof awt_Component.cpp > - Formatting awt_Component.cpp > - Merge branch 'openjdk:master' into patch-10 > - Update src/java.desktop/windows/native/libawt/windows/awt_Component.cpp > >Co-authored-by: Alexey Ivanov > - 8334599 Weird syntax to someone like me, but I can see the benefit. Using memmove is so uncommon that it is usually a clear indication for a deliberate choice. - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19798#pullrequestreview-2158913616
Re: RFR: 8334599: Improve code from JDK-8302671 [v2]
On Wed, 3 Jul 2024 13:25:27 GMT, Julian Waters wrote: >> 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. > > I can't quite comment on that since I don't really know what the purpose of > the memmove is. What does @prrace think? Both `memcpy` and `memmove` do the same thing, namely each function copies bytes from one location into another location. The difference between these two functions is in whether buffers are allowed to overlap or not: * [`memcpy`](https://en.cppreference.com/w/c/string/byte/memcpy): https://en.cppreference.com/w/c/string/byte/memcpy;>If the objects overlap […], the behavior is undefined.https://en.cppreference.com/w/c/string/byte/memcpy#Notes;>`memcpy` is the fastest library routine for memory-to-memory copy. It is usually more efficient than `strcpy`, which must scan the data it copies or `memmove`, which must take precautions to handle overlapping inputs. * [`memmove`](https://en.cppreference.com/w/c/string/byte/memmove): https://en.cppreference.com/w/c/string/byte/memmove;>The objects may overlap: copying takes place as if the characters were copied to a temporary character array and then the characters were copied from the array to `dest`. > we know for sure that `tmpState` and `kstate` do not overlap. For this reason, `memcpy` is safe to use, and it is more efficient than `memmove`. To be clear, I'm not proposing to incorporate this change under this bug. I just pointed out a possible optimisation. - PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1665654705
Re: RFR: 8334599: Improve code from JDK-8302671 [v2]
> 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 Julian Waters 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 five additional commits since the last revision: - Formatting sizeof awt_Component.cpp - Formatting awt_Component.cpp - Merge branch 'openjdk:master' into patch-10 - Update src/java.desktop/windows/native/libawt/windows/awt_Component.cpp Co-authored-by: Alexey Ivanov - 8334599 - Changes: - all: https://git.openjdk.org/jdk/pull/19798/files - new: https://git.openjdk.org/jdk/pull/19798/files/8f5f3088..b98068e9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19798=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19798=00-01 Stats: 19745 lines in 608 files changed: 11255 ins; 6128 del; 2362 mod Patch: https://git.openjdk.org/jdk/pull/19798.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19798/head:pull/19798 PR: https://git.openjdk.org/jdk/pull/19798
Re: RFR: 8334599: Improve code from JDK-8302671
On Mon, 24 Jun 2024 20:43:00 GMT, Alexey Ivanov 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 > > 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. I can't quite comment on that since I don't really know what the purpose of the memmove is. What does @prrace think? - PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1664191137
Re: RFR: 8334599: Improve code from JDK-8302671
On Fri, 28 Jun 2024 00:56:48 GMT, Julian Waters wrote:
>> src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3366:
>>
>>> 3364: static void
>>> 3365: resetKbdState( BYTE ()[256]) {
>>> 3366: BYTE tmpState[256];
>>
>> Suggestion:
>>
>> resetKbdState( BYTE ()[AwtToolkit::KB_STATE_SIZE]) {
>> BYTE tmpState[AwtToolkit::KB_STATE_SIZE];
>>
>> Will this resolve Phil's concern? Both arrays will use the same size.
>
>> Will this resolve Phil's concern? Both arrays will use the same size.
>
> @prrace Does this address your concerns? If it does I will commit it
That's fine since that what I was asking for in my comment a week ago
> Why can't we EVERYWHERE use the definition used to create the array passed in
> KB_STATE_SIZE = 256 instead of a mixture.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1659284190
Re: RFR: 8334599: Improve code from JDK-8302671
On Mon, 24 Jun 2024 20:40:54 GMT, Alexey Ivanov wrote: > Will this resolve Phil's concern? Both arrays will use the same size. @prrace Does this address your concerns? If it does I will commit it - PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1657972007
Re: RFR: 8334599: Improve code from JDK-8302671
On Thu, 20 Jun 2024 08:29:39 GMT, Julian Waters 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 ()[256]) {
> 3366: BYTE tmpState[256];
Suggestion:
resetKbdState( BYTE ()[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
Re: RFR: 8334599: Improve code from JDK-8302671
On Thu, 20 Jun 2024 08:29:39 GMT, Julian Waters 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 Thanks for the review. But I'd like to address your other concerns too if I can - PR Comment: https://git.openjdk.org/jdk/pull/19798#issuecomment-2183795151
Re: RFR: 8334599: Improve code from JDK-8302671
On Thu, 20 Jun 2024 08:29:39 GMT, Julian Waters 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 OK .. just do what you have. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19798#pullrequestreview-2133487804
Re: RFR: 8334599: Improve code from JDK-8302671
On Thu, 20 Jun 2024 08:29:39 GMT, Julian Waters 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 Yes, it doesn't change any existing behaviour, it's just a more robust way of expressing what was already there before, so it should be harmless. I'm not sure I follow your other concern about it being a mixture though, I checked and this method is static and only used in this file, and the few callsites that do use it all initialize their arrays to KB_STATE_SIZE. Do you mean getting rid of the hardcoded 256 in this method's array parameter? The other thing about passing an array by reference means if the caller doesn't get the array size exactly right, it will not result in a silent bug, it is an immediate compilation error, so if the caller passes a shorter array it will simply refuse to compile, which is likely what we want here - PR Comment: https://git.openjdk.org/jdk/pull/19798#issuecomment-2181982129
Re: RFR: 8334599: Improve code from JDK-8302671
On Thu, 20 Jun 2024 08:29:39 GMT, Julian Waters 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 Harmless, I hope, but it doesn't change that 256 is hard-coded in multiple places AND that the caller has to get it right too .. if the caller passes a shorter array things can still go wrong. Why can't we EVERYWHERE use the definition used to create the array passed in KB_STATE_SIZE = 256 instead of a mixture. - PR Review: https://git.openjdk.org/jdk/pull/19798#pullrequestreview-2131336310
RFR: 8334599: Improve code from JDK-8302671
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 - Commit messages: - 8334599 Changes: https://git.openjdk.org/jdk/pull/19798/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19798=00 Issue: https://bugs.openjdk.org/browse/JDK-8334599 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19798.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19798/head:pull/19798 PR: https://git.openjdk.org/jdk/pull/19798
