On Tue, 14 Mar 2023 20:47:55 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed method descriptions
>
> make/jdk/src/classes/build/tools/generatecharacter/EmojiData.java line 99:
>
>> 97: case "Emoji_Component" -> EMOJI_COMPONENT;
>> 98: case "Extended_Pictographic" -> EXTENDED_PICTOGRAPHIC;
>> 99: default -> throw new InternalError();
>
> It would be useful to include the "type" as the exception argument. It give
> some idea as to the corruption or missing case.
Added `type` to the error message
> make/jdk/src/classes/build/tools/generatecharacter/GenerateCharacter.java
> line 214:
>
>> 212: maskEmojiModifierBase = 0x020000000000L,
>> 213: maskEmojiComponent = 0x040000000000L,
>> 214: maskExtendedPictographic = 0x080000000000L;
>
> It would be good to leverage a common definition (perhaps a bit number) here
> and in EmojiData.java
> and build the constants with <<< shifts.
Good point. I managed to get rid of the constants in `EmojiData` altogether, by
using the constants in `GenerateCharacter`. Used the bit numbers to construct
constants.
> make/jdk/src/classes/build/tools/generatecharacter/GenerateCharacter.java
> line 810:
>
>> 808: if (x.equals("maskEmojiModifierBase")) return "0x" +
>> hex4(maskEmojiModifierBase >> 32);
>> 809: if (x.equals("maskEmojiComponent")) return "0x" +
>> hex4(maskEmojiComponent >> 32);
>> 810: if (x.equals("maskExtendedPictographic")) return "0x" +
>> hex4(maskExtendedPictographic >> 32);
>
> An upgrade would be to modify hex4(), hexNN() to use
> `HexFormat.of().toUpperCase().toHexDigits((short)xxx)`
> The HexFormat is reusable and would avoid creating extra strings.
> Perhaps also create a method that combines the repetitive shift and prefixing.
>
> This if...then... sequence could be an expression switch (x) {...}.
It would be good, but it would be for another day IMO.
-------------
PR: https://git.openjdk.org/jdk/pull/13006
