[foreign-memaccess+abi] RFR: 8291639: Improve the ability to visualize a MemorySegment in human readable forms [v2]

[Maurizio Cimadamore]

On Fri, 26 May 2023 13:01:56 GMT, Rémi Forax <forax at openjdk.org> wrote:

>> Per Minborg has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 29 commits:
>> 
>>  - Merge master
>>  - Fix typos
>>  - Clean up toString methods
>>  - Add support for MemoryAddress arrays and improve docs
>>  - Improve javadoc
>>  - Change to IAE if mapper fails
>>  - Remove caching
>>  - Add tests for initial issue examples
>>  - Clean up and optimize
>>  - Add support for multidimensional records
>>  - ... and 19 more: https://git.openjdk.org/panama-foreign/compare/bee4503b...1d7db589
>
> src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 209:
> 
>> 207:      * @since 21
>> 208:      */
>> 209:     <R extends Record> Function<MemorySegment, R> recordMapper(Class<R> type);
> 
> you also need to throw an IAE if type is Record.class, java.lang.Record is the only subtype of java.lang.Record which is not a record

I wonder if an overload which takes a MethodHandles.Lookup might help with the accessibility issues (but we can add that later)

> src/java.base/share/classes/jdk/internal/foreign/LayoutRecordMapper.java line 108:
> 
>> 106:             throw new IllegalArgumentException("There is no mapping for " +
>> 107:                     missingComponents + " in " + type.getName() +
>> 108:                     "(" + String.join(", ", recordComponents.stream().map(RecordComponent::getName).collect(Collectors.joining(", "))) + ")" +
> 
> Two joins, String.join and Collectors.joining() ??

Also, I think this can be simplified - e.g. "there is no mapping for [list of component names] in [group layout]". E.g. just printing the group layout will be enough, I think.

> src/java.base/share/classes/jdk/internal/foreign/LayoutRecordMapper.java line 128:
> 
>> 126:                     long byteOffset = layout.byteOffset(pathElement) + offset;
>> 127: 
>> 128:                     return switch (cl.layout()) {
> 
> this switch is too big, there is even a switch in that switch, which makes the code hard to read

I agree - ideally, each case should be split into its own method. That should help recursion too.

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

PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1206863171
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1206868438
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1206875677