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

[Jayathirth D V]

On Wed, 25 Oct 2023 23:42:08 GMT, Phil Race <prr at 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