On Wed, 6 Dec 2023 14:09:31 GMT, Maurizio Cimadamore <[email protected]>
wrote:
>> src/java.base/share/classes/java/lang/invoke/X-VarHandleSegmentView.java.template
>> line 153:
>>
>>> 151: static void set(VarHandle ob, Object obb, long base, $type$ value)
>>> {
>>> 152: VarHandleSegmentViewBase handle = (VarHandleSegmentViewBase)ob;
>>> 153: AbstractMemorySegmentImpl bb = checkAddress(obb, base,
>>> handle.length, ON_WRITE_UOE);
>>
>> I don't think this should throw UOE. It should throw IAE, because the bad
>> segment is passed to the var handle. I also realize that this exception is
>> not captured in the documentation of MemoryLayout::varHandle. So that should
>> be fixed as well.
>
> This is actually tricky: for var handles it seems like the right exception is
> IAE - but then for MemorySegment::get the right exception is UOE (because the
> receiver itself is read-only). We might need to give up consistency a bit, as
> otherwise I don't think memory segment getter/setter can reuse the VarHandle
> implementation.
After offline discussion: the distinction between UOE and IAE is mostly
fictional and doesn't add much. In fact, such distinction is harmful because it
would mean that code written using var handles cannot cleanly migrate to use
memory segment getter/setter (and vice-versa). As such, I recommend that the
implementation is left alone, and IAE is thrown by the var handle accessors for
read-only segments. Memory segment setters should similarly throw IAE for
read-only, because the setters are just thin easy to use wrappers around the
var handle.
A similar argument holds for MemorySegment::copyFrom - here throwing UOE would
be more apt, but given we explain this method as calling the static
MemorySegment::copy (which throws IAE) I would again avoid exception mismatch
which would be detrimental to refactoring.
At which point we're left with MemorySegment::fill. While UOE would be better
here, it would be the only method in MS issuing UOE for a read-only issue, so I
think it would be just best to regularize and use IAE here too.
tl;dr; the implementation is already throwing the correct set of exceptions
(which is not perfect, but better than the alternatives). The javadoc should be
updated to reflect that.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16993#discussion_r1417426024
