[foreign-memaccess+abi] RFR: 8293510: Code style issues in the foreign API [v2]

[Maurizio Cimadamore]

On Fri, 16 Sep 2022 08:07:37 GMT, Per Minborg <duke at openjdk.org> wrote:

>> This PR fixes numerous code style issues. PRs like this are work intensive to review and contain subjective improvements which could be debated. I think it is better to make these changes now rather than later.
>
> Per Minborg has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits:
> 
>  - Merge foreign-memaccess+abi
>  - Fix import
>  - Merge with foreign-memaccess+abi
>  - Break out functionality into Utils
>  - Reformat public interface declartions
>  - Cleanup method parameters in internal classes
>  - Cleanup method parameters in public classes
>  - Rearrange code
>  - Fix naming issue
>  - Merge with foreign-memaccess+abi
>  - ... and 5 more: https://git.openjdk.org/panama-foreign/compare/3e8ef17b...d8294460

As said in one of the comments, we should try to separate out the various changes. I did a pass on the code, by filtering out one commit at a time, which was useful. What I liked:

* general better consistency on use of `final`
* parameter naming consistency
* moving some of the bigger methods `copy` `mismatch` out (although I think the destination should be different)
* use of `record` in places where we didn't use them
* breaking up long method call chains
* fixing of imports

What I find more questionable:

* alphabetically reordering all members
* breaking apart method parameters no matter what

There is also one API change (removal of MemoryLayout::valueLayout) which is a red herring in a code style PR.

src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 282:

> 280:     }
> 281: 
> 282:     private static <Z> Z computePathOp(LayoutPath path,

I'm generally not too sure about this. Yes, if there's a lot of code it might be useful to move it out - but in this case it's a single method used in several other methods in this interface. Why is it bad to use a private static interface method for something like this?

src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 636:

> 634:      * @throws IllegalArgumentException if the carrier type is not supported.
> 635:      */
> 636:     static ValueLayout valueLayout(Class<?> carrier, ByteOrder order) {

This method should not be removed. It is very useful when working with layouts programmatically (e.g. when a client wants to create a value layout from a class carrier). Even if it's unused in code (but it is used in one test), we should not treat this factory as useless. Also, I was only able to find this by looking at commits one by one. So, as a general comment, perhaps it would be a good idea to split the various changes.

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1894:

> 1892:         Objects.requireNonNull(dstElementLayout);
> 1893: 
> 1894:         Utils.copy(srcSegment, srcElementLayout, srcOffset, dstSegment, dstElementLayout, dstOffset, elementCount);

I think it would make more sense to move the `copy` method to AbstractMemorySegmentImpl - where other helper stuff for segment is defined.

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 2016:

> 2014:         Objects.requireNonNull(dstSegment);
> 2015: 
> 2016:         return Utils.mismatch(srcSegment,srcFromOffset,srcToOffset, dstSegment, dstFromOffset, dstToOffset);

Same here - mismatch should live in AbstractMemorySegment

src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 345:

> 343:      * or if {@code alignmentBytes} is not a power of 2.
> 344:      */
> 345:     MemorySegment allocate(long byteSize, long byteAlignment);

Good stuff!

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 77:

> 75: 
> 76:     @ForceInline
> 77:     AbstractMemorySegmentImpl(long length,

Here I'm a bit puzzled. Looking at other JDK code, I don't see this rule being applied consistently. I think I'm ok with breaking up long parameter lists (an example of that below) - but doing that for all method decls seems a bit too much IMHO.

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 138:

> 136:      * Mismatch over long lengths.
> 137:      */
> 138:     public static long vectorizedMismatchLargeForBytes(MemorySessionImpl aSession,

So, this is a decision whether to break up one per line, or break up so that we never exceed 100 chars per line. I don't have any strong opinion here, but I note that the JDK code is not consistent about this at all.

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 243:

> 241:     @Override
> 242:     public void load() {
> 243:         throw newUnsupportedOperationExceptionNotAMappedSegment();

Nice

src/java.base/share/classes/jdk/internal/foreign/abi/VMStorage.java line 38:

> 36:  * @param debugName         the debug name to use
> 37:  */
> 38: public record VMStorage(byte type,

This is well spotted - please separate it out

src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/CallArranger.java line 81:

> 79: 
> 80:     // record
> 81:     public record Bindings(

Also good

test/jdk/java/lang/Thread/jni/AttachCurrentThread/ImplicitAttach.java line 65:

> 63:         // void start_threads(int count, void *(*f)(void *))
> 64:         SymbolLookup symbolLookup = SymbolLookup.loaderLookup();
> 65:         MemorySegment symbol = symbolLookup.find("start_threads").orElseThrow();

This is dealt with in another PR: https://github.com/openjdk/panama-foreign/pull/722

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

PR: https://git.openjdk.org/panama-foreign/pull/721