RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout [v4]

[Phil Race]

On Wed, 15 Nov 2023 15:06:55 GMT, Jayathirth D V <jdv at openjdk.org> wrote:

>> Phil Race has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   indentation
>
> src/java.desktop/share/classes/sun/font/HBShaper.java line 66:
> 
>> 64:      * };
>> 65:      */
>> 66:    private static final UnionLayout VarIntLayout = MemoryLayout.unionLayout(
> 
> Indentation issue.

fixed

> src/java.desktop/share/classes/sun/font/HBShaper.java line 84:
> 
>> 82:      * };
>> 83:      */
>> 84:      private static final StructLayout PositionLayout = MemoryLayout.structLayout(
> 
> Indentation issue.

fixed

> src/java.desktop/share/classes/sun/font/HBShaper.java line 144:
> 
>> 142:     private static final MemorySegment store_layout_results_stub;
>> 143: 
>> 144:    private static FunctionDescriptor
> 
> Indentation issue.

fixed

> src/java.desktop/share/classes/sun/font/HBShaper.java line 153:
> 
>> 151:    }
>> 152: 
>> 153:    private static MethodHandle getMethodHandle
> 
> Indentation issue.

fixed

> src/java.desktop/share/classes/sun/font/HBShaper.java line 165:
> 
>> 163:    }
>> 164: 
>> 165:    static {
> 
> Indentation issue.

fixed

> src/java.desktop/share/classes/sun/font/HBShaper.java line 212:
> 
>> 210:         jdk_hb_shape_handle = tmp4;
>> 211: 
>> 212:         Arena garena = Arena.global(); // creating stubs that exist until VM exit.
> 
> Variable name `gArena`(for global arena) is better than using `garena`.

i'm using lowercase in the other names, so I think it OK

> src/java.desktop/share/classes/sun/font/HBShaper.java line 347:
> 
>> 345:         glyphIDPtr.setAtIndex(JAVA_INT, 0, glyphID);
>> 346:         return (glyphID != 0) ? 1 : 0;
>> 347: }
> 
> Indentation issue.

fixed

> src/java.desktop/share/classes/sun/font/HBShaper.java line 659:
> 
>> 657:         startPt.y = advY;
>> 658:         startPt.x = advX;
>> 659:   }
> 
> Indentation issue.

fixed

> src/java.desktop/share/classes/sun/font/SunLayoutEngine.java line 193:
> 
>> 191:         } else {
>> 192:            long pFace = getFacePtr(font);
>> 193:             shape(font, strike, ptSize, mat, pFace,
> 
> Is it okay to not have `(pFace != 0)` check here?

i added back the check

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1396267684
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1396267831
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1396268039
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1396268116
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1396268358
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1396268796
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1396269031
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1396269300
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1396267142