[foreign-memaccess+abi] RFR: 8264515: Rename static factory methods in the foreign API [v2]

[Paul Sandoz]

On Wed, 31 Mar 2021 15:20:55 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> It has been pointed out recently, that some of the static factory names are not very friendly with use of static imports. The latest case is `ResourceScope::ofImplicit`, which would lead to code like this:
>> 
>> MemorySegment.allocateNative(100, ofImplicit());
>> 
>> But there's also the old good layout API:
>> 
>> MemoryLayout layout = MemoryLayout.ofSequence(10,
>>                                   MemoryLayout.ofStruct(
>>                                              MemoryLayout.ofValueBits(4).withName("x"),
>>                                              MemoryLayout.ofPadding(8),
>>                                              MemoryLayout.ofValueBits(4).withName("y"),
>>                                   )); 
>> 
>> We've been examining precedents, and also looking at some of the existing comments on the topic [1], and we came up with the following principles, which we have tried to adhere to in our renaming effort:
>> 
>> 
>> * a static factory should have a name that stands on its own (think `MethodType.methodType`)
>> * in some cases, when converting between different things, or when creating something out of something else the prefix `of` is acceptable (`LocalDateTime.ofInstant`); this also means static imports cannot be used in these cases
>> * if returning a new instance is relevant semantic-wise, factory name should be prefixed with `new` (as in `Files.newInputStream`); otherwise `new` can be omitted
>> 
>> Armed with these principles, we have made the following changes to the API:
>> 
>> ### MemoryLayouts
>> 
>> Factories have been renamed from `ofXYZ` to `XYZLayout` - so the above example becomes:
>> 
>> MemoryLayout layout = sequenceLayout(10,
>>                                   structLayout(
>>                                              valueLayout(4).withName("x"),
>>                                              paddingLayout(8),
>>                                              valueLayout(4).withName("y"),
>>                                   )); 
>> 
>> Which is much more readable.
>> 
>> ### ResourceScope
>> 
>> Here we opted for the following mapping:
>> 
>> * ResourceScope::ofImplicit -> ResourceScope::newImplicitScope
>> * ResourceScope::ofConfined -> ResourceScope::newConfinedScope
>> * ResourceScope::ofShared -> ResourceScope::newSharedScope
>> * ResourceScope::globalScope ->  ResourceScope::globalScope (unchanged)
>> 
>> So, the above example becomes:
>> 
>> MemorySegment.allocateNative(100, newImplicitScope());
>> 
>> Again, more readable and explicit.
>> 
>> ### SegmentAllocator
>> 
>> Here it's tricky, as we have a mix of allocator-sounding names (`arenaBounded`/`arenaUnbounded`/`malloc`) and some odd ones out (`scoped`, `prefix`, `implicit`).
>> 
>> So, in the case of `arenaBounded`/`arenaUnbounded`, we think that having a couple of `arenaAllocator` overloads would be preferrable.
>> 
>> As for `scoped` and `prefix`, we think it's more useful to think of these as *conversions*, so use `of`. Following the precedent on `LocalDateTime`, let's also add the type name aftre `of`, e.g. `ofScope` and `ofSegment`, respectively.
>> 
>> This leaves us with `malloc` and `implicit`. Now, I think while `malloc` is allocator sounding, it is also a misleading name. This is really an allocator that just ends up calling `MemorySegment::allocateNative` with fresh scopes. 
>> 
>> We also noted that, being `SegmentAllocator` a functional interface, the gain with using `malloc` is relatively small:
>> 
>> SegemtAllocator allocator = malloc(ResourceScope::newConfinedScope);
>> 
>> vs.
>> 
>> SegmentAllocator allocator = (size, align) -> MemorySegment.allocateNative(size, align, newConfinedScope())
>> 
>> So we opted to remove `malloc`; we also applied a similar argument for the implicit allocator (which, in fact, is really like `malloc(ResourceScope::newImplicitScope)`). So, to summarize:
>> 
>> * SegmentAllocator::arenaBounded -> ResourceScope::arenaAllocator
>> * SegmentAllocator::arenaUnbounded -> ResourceScope::arenaAllocator
>> * SegmentAllocator::prefix -> ResourceScope::ofSegment
>> * SegmentAllocator::scoped -> ResourceScope::ofScope
>> * SegmentAllocator::malloc -> // removed
>> * SegmentAllocator::implicit -> // removed
>> 
>> ### Conclusions
>> 
>> While (many) other naming schemes are possible, what we did seems consistent with precedent set by other JDK API, and seems to land the API in a place which is more amenable to using static imports, which seems like an improvement (gievn the number of static methods in the API).
>> 
>> While most of the changes were done using the IDE, some of them have been done manually - specifically those fixing the currently broken micro benchmark (after removal of "restricted" suffix yesterday), and fixes to scope-less MemorySegment::allocateNative in various javadoc samples.
>> 
>> [1] - https://blog.joda.org/2011/08/common-java-method-names.html
>
> Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge branch 'foreign-memaccess+abi' into static-factory-rename
>  - Rename static factories
>    Fix usage of default factories in micro benchmakrs and in javadoc samples

Very good.

Something to ponder after this PR. I have my suspicion that the distinction with the "of" naming convention will be lost on many as such methods get swept up in the `static import ...SegmentAllocator.*`. It may be better to be simpler (less nuanced) and consistent.

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

Marked as reviewed by psandoz (Committer).

PR: https://git.openjdk.java.net/panama-foreign/pull/487