On Wed, 25 Oct 2023 23:42:08 GMT, Phil Race <p...@openjdk.org> wrote:
>> 8318364: Add an FFM-based implementation of harfbuzz OpenType layout
>
> 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.
src/java.desktop/share/classes/sun/font/HBShaper.java line 84:
> 82: * };
> 83: */
> 84: private static final StructLayout PositionLayout =
> MemoryLayout.structLayout(
Indentation issue.
src/java.desktop/share/classes/sun/font/HBShaper.java line 142:
> 140: private static final MemorySegment get_contour_pt_stub;
> 141:
> 142: private static final MemorySegment store_layout_results_stub;
I see this Upcall is named `store_layout_results` in case of FFM and we call it
similar function in JNI case as `storeGVData`. If we can find some common name
and use it will be beneficial in future when we want to compare code between
FFM and JNI implementation.
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.
src/java.desktop/share/classes/sun/font/HBShaper.java line 153:
> 151: }
> 152:
> 153: private static MethodHandle getMethodHandle
Indentation issue.
src/java.desktop/share/classes/sun/font/HBShaper.java line 165:
> 163: }
> 164:
> 165: static {
Indentation issue.
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`.
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.
src/java.desktop/share/classes/sun/font/HBShaper.java line 659:
> 657: startPt.y = advY;
> 658: startPt.x = advX;
> 659: }
Indentation issue.
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?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1394338584
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1394339310
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1394403920
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1394339849
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1394340526
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1394341308
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1394370065
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1394342419
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1394345302
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1394293925
