On Mon, 5 May 2025 04:50:30 GMT, Chen Liang <li...@openjdk.org> wrote:
>> Credit to @lukellmann that the duplication arg handling in #24742 avoided
>> throwing exceptions but produced a wrong option. This patch fixes that and
>> removed stream usages in CaptureCallState to speed up bootstrap.
>>
>> Also, the previous patch affected the toString display of the option; I
>> added a unit test to ensure the option prints names that is user-friendly.
>>
>> Another thing I noted is `CapturableState` uses `OperatingSystem`; using
>> `valueOf` brings a performance overhead due to setups with reflection, so I
>> made this lazy. (The enum is thread safe, so we allow racy access to the
>> cache field)
>>
>> Testing: jdk/lang/foreign, tier 1-3 in progress.
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> No env to test
Note that the use of the unsequenced `Map.of(…)` results in the return value of
`CapturableState.displayString(…)` to no longer be ordered by the mask bits.
To fix this, simply add a `private static List<CapturableState>`[^1] for
storing the supported values:
[^1]: And/or introduce and use unmodifiable `SequencedMap.of(…)` and
`SequencedSet.of(…)` factory methods.
src/java.base/share/classes/jdk/internal/foreign/abi/CapturableState.java line
55:
> 53: } else {
> 54: supported = List.of(new CapturableState("errno", JAVA_INT, 1
> << 2));
> 55: }
Suggestion:
private static final List<CapturableState> SUPPORTED;
static {
final List<CapturableState> supported;
if (OperatingSystem.isWindows()) {
supported = List.of(
new CapturableState("GetLastError", JAVA_INT, 1 << 0),
new CapturableState("WSAGetLastError", JAVA_INT, 1 << 1),
new CapturableState("errno", JAVA_INT, 1 << 2)
);
} else {
supported = List.of(new CapturableState("errno", JAVA_INT, 1 << 2));
}
SUPPORTED = supported;
src/java.base/share/classes/jdk/internal/foreign/abi/CapturableState.java line
96:
> 94: public static String displayString(int mask) {
> 95: var displayList = new ArrayList<>();
> 96: for (var e : LOOKUP.values()) {
Suggestion:
for (var e : SUPPORTED) {
-------------
PR Review: https://git.openjdk.org/jdk/pull/25025#pullrequestreview-2813828585
PR Review Comment: https://git.openjdk.org/jdk/pull/25025#discussion_r2072860691
PR Review Comment: https://git.openjdk.org/jdk/pull/25025#discussion_r2072860830
