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

Maurizio Cimadamore mcimadamore at openjdk.java.net
Wed Mar 31 14:31:52 UTC 2021


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

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

Commit messages:
 - Rename static factories

Changes: https://git.openjdk.java.net/panama-foreign/pull/487/files
 Webrev: https://webrevs.openjdk.java.net/?repo=panama-foreign&pr=487&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264515
  Stats: 725 lines in 94 files changed: 6 ins; 73 del; 646 mod
  Patch: https://git.openjdk.java.net/panama-foreign/pull/487.diff
  Fetch: git fetch https://git.openjdk.java.net/panama-foreign pull/487/head:pull/487

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


More information about the panama-dev mailing list