On Tue, 17 Oct 2023 11:50:20 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> 8318364: Add an FFM-based implementation of harfbuzz OpenType layout
>
> src/java.desktop/share/classes/sun/font/HBShaper.java line 310:
>
>> 308: SequenceLayout glyphInfosLayout =
>> MemoryLayout.sequenceLayout(maxinfo, GlyphInfoLayout);
>> 309: codePointHandle = getVarHandle(glyphInfosLayout, "codepoint");
>> 310: clusterHandle = getVarHandle(glyphInfosLayout, "cluster");
>
> There are better ways to do this in the latest API, by using the extra offset
> parameter and `MemoryLayout::scaleHandle`.
>
> I suggest changing this to:
>
> Suggestion:
>
> x_offsetHandle = getVarHandle(PositionLayout, "x_offset");
> y_offsetHandle = getVarHandle(PositionLayout, "y_offset");
> x_advanceHandle = getVarHandle(PositionLayout, "x_advance");
> y_advanceHandle = getVarHandle(PositionLayout, "y_advance");
> codePointHandle = getVarHandle(GlyphInfoLayout, "codepoint");
> clusterHandle = getVarHandle(GlyphInfoLayout, "cluster");
>
>
> And then in `getVarHandle`:
>
>
> private static VarHandle getVarHandle(MemoryLayout enclosing, String
> name) {
> VarHandle h = layout.varHandle(PathElement.groupElement(name));
> // scale offset by the size of 'enclosing'
> h = MethodHandles.collectCoordinates(h, 1, enclosing.scaleHandle());
> /* insert 0 offset so don't need to pass arg every time */
> return MethodHandles.insertCoordinates(h, 1,
> 0L).withInvokeExactBehavior();
> }
>
>
> (hope I eyeballed that correctly...)
I've just pushed this change, but I'm unclear why it is "better". It seems more
obscure to me.
I've added a somewhat expanded comment of what I guess this does to help
me and others understand what it does. Please take a look to see if it makes
sense.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1362900089
