On Mon, 15 Jul 2024 14:09:22 GMT, Chen Liang <li...@openjdk.org> wrote:
>> The `@Stable` on the `index` field is incorrect, as stable only avoids
>> inlining `0`. Solution is to use bit flip on the actual index (and rename
>> the field to `flippedIndex`), so we use -1 to -256 (mapping to 0 to 255) and
>> 0 the default value is used as an unset indicator.
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> We have sufficient space in short, use +1 offset
Looks reasonable to me, with nits.
src/java.base/share/classes/java/lang/invoke/LambdaForm.java line 1400:
> 1398: if (offsetIndex != i + 1) {
> 1399: if (offsetIndex != 0) return false;
> 1400: offsetIndex = (short) (i + 1);
Can we pull `(short) (i + 1);` into a local variable, given that we need it on
all paths?
src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 778:
> 776: var newParameters = new TreeMap<Name, Integer>(new
> Comparator<>() {
> 777: public int compare(Name n1, Name n2) {
> 778: return n1.index() - n2.index();
We don't have to call the method here and do the translation to "proper" index
here, or do we?
-------------
Marked as reviewed by shade (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20178#pullrequestreview-2177881698
PR Review Comment: https://git.openjdk.org/jdk/pull/20178#discussion_r1677980731
PR Review Comment: https://git.openjdk.org/jdk/pull/20178#discussion_r1677926814
