On Thu, 12 May 2022 15:45:01 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> 
wrote:

>> This PR contains the API and implementation changes for JEP-424 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request with a new target base due 
> to a merge or a rebase. The pull request now contains 65 commits:
> 
>  - Merge branch 'master' into foreign-preview
>  - Merge branch 'master' into foreign-preview
>  - Fix crashes in heap segment benchmarks due to misaligned access
>  - Merge branch 'master' into foreign-preview
>  - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java
>    
>    Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>  - Add tests for loaderLookup/restricted method corner cases
>  - * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI
>    * Tweak restricted method check to work when there's no caller class
>  - Tweak examples in SymbolLookup javadoc
>  - Merge branch 'master' into foreign-preview
>  - Update 
> src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
>    
>    Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>  - ... and 55 more: 
> https://git.openjdk.java.net/jdk/compare/82aa0455...f170415b

Some late comments and suggestions to fix inconsistencies and wrong javadocs.

src/java.base/share/classes/java/nio/channels/FileChannel.java line 1045:

> 1043:      *
> 1044:      * @throws UnsupportedOperationException
> 1045:      *         If an unsupported map mode is specified.

I think this should mention that UOE is also throws if the filesystem 
implementation does not support memory mapping. This may happen for filesystems 
like jrtfs or custom wrapper filesystems that do not implement the method below.

As this is already merged, should I open a PR fixing the docs or open an issue?

src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 1164:

> 1162:         }
> 1163:         if (unmapper != null) {
> 1164:             AbstractMemorySegmentImpl segment = new 
> MappedMemorySegmentImpl(unmapper.address(), unmapper, size,

When reviewing the method for MappedByteBuffer: I think to make this consistent 
the "old" method should also use `address()` which applies the pagePosition. 
Currently it is confusing:
- New code returning `MemorySegment` uses `unmapper.address()`
- Old code returning `MappedByteBuffer` uses `unmapper.address + 
unmapper.pagePosition` (fields)

Should I open an issue or a PR to fix this (because this is already merged)?

See the mailing list posts:
- https://mail.openjdk.java.net/pipermail/panama-dev/2022-May/016981.html
- https://mail.openjdk.java.net/pipermail/panama-dev/2022-May/016990.html

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

PR: https://git.openjdk.java.net/jdk/pull/7888

Reply via email to