[foreign-memaccess+abi] RFR: 8291473: Unify MemorySegment and MemoryAddress [v2]

[Maurizio Cimadamore]

On Thu, 28 Jul 2022 11:48:57 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with four additional commits since the last revision:
>> 
>>  - Update src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
>>    
>>    Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>
>>  - Update src/java.base/share/classes/java/lang/foreign/VaList.java
>>    
>>    Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>
>>  - Update src/java.base/share/classes/java/lang/foreign/ValueLayout.java
>>    
>>    Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>
>>  - Update src/java.base/share/classes/java/lang/foreign/package-info.java
>>    
>>    Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>
>
> src/java.base/share/classes/java/lang/foreign/VaList.java line 247:
> 
>> 245:      */
>> 246:     @CallerSensitive
>> 247:     static VaList ofAddress(long address, MemorySession session) {
> 
> Don't we still want the native access (and `null`)  checks here? I see that the VaList impls eventually end up calling `MemorySegment.ofAddress` any way, but I think it's clearer to check it here as well (both in terms of code readability, as well as stack traces that are produced).

Whoops - yes. This is the result on some back and forth on my part that I forgot to restore.

> src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 248:
> 
>> 246:         throw new UnsupportedOperationException("Cannot compute offset from native to heap (or vice versa).");
>> 247:     }
>> 248: 
> 
> To be honest. I would prefer to save the removal of these methods for another patch.
> 
> The methods were added in the first place, so I'm not so sure that they are no longer needed now. ~Even without the `array()` accessor, it was possible for clients to implement these methods using `isNative` in the past.~
> 
> Having a separate patch will also make it easier to find this specific change, the reasoning behind it, and potentially weigh in on it.

Fair point, I can separate out. As you noted, though, presence of array() and totalization of address() does change things considerably in this space. We added these methods because they could not have been written by hand. But dealing with that in a separate patch sounds fine.

> src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java line 72:
> 
>> 70:     public long maxAlignMask() {
>> 71:         return 0;
>> 72:     }
> 
> I don't think this implementation of `maxAlignMask` is needed? All the implementations override it already.

I see - odd.

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

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