Covariant overrides on the Buffer Hierachy

[Peter Levart]

On 04/20/2014 08:56 PM, Richard Warburton wrote:
> Hi,
>
> It looks to me like there are no obvious problems with changing the buffer
>> subclasses to use covariant overrides. As Alan mentioned, one API was
>> modified this way in 7, by Martin Buchholz. The discussion threads for this
>> change are [2] and [3]. It looks like Martin considered additional
>> covariant overrides, but ultimately backed off from this for a variety of
>> incidental reasons; see [4]. There is a certain amount of work here above
>> and beyond just changing the return types, but there don't appear to be any
>> fundamental issues.
> So I've put a patch on cr at
> http://cr.openjdk.java.net/~rwarburton/buffer-overrides-0/

Hi,

Since the public part of Buffer type hierarchy is (mostly) just 
two-level, an alternative would be to generify java.nio.Buffer:

public abstract class Buffer<B extends Buffer<B>> {

     @SuppressWarnings("unchecked")
     protected final B self() { (B) return this; }

public final B position(int newPosition) {
         ...
         return self();
     }
     ...
}

public abstract class LongBuffer
     extends Buffer<LongBuffer>
     implements Comparable<LongBuffer>
{
...
}

public abstract class ShortBuffer
     extends Buffer<ShortBuffer>
     implements Comparable<ShortBuffer>
{
...
}

...

The question is what to do with ByteBuffer/MappedByteBuffer then. One 
alternative is to fix the type variable constraint at ByteBuffer level 
and leave MappedByteBuffer astray:

public abstract class ByteBuffer
     extends Buffer<ByteBuffer>
     implements Comparable<ByteBuffer>
{
...
}

public abstract class MappedByteBuffer
     extends ByteBuffer
{
...
}


...in this case Buffer/ByteBuffer methods invoked on MappedByteBuffer 
would have ByteBuffer return type. This would only matter if one wanted 
to call MappedByteBuffer's methods at the end of the chain - there are 
three of them: isLoaded()/load()/force().

The other alternative is to fix the type variable constraint at 
MappedByteBuffer level:

public abstract class ByteBuffer
     extends Buffer<B extends ByteBuffer<B>>
     implements Comparable<ByteBuffer<?>>
{
...
}

public abstract class MappedByteBuffer
     extends ByteBuffer<MappedByteBuffer>
{
...
}


...in this case, the following:

     ByteBuffer bb = ...;

would be a raw type and consequently Buffer methods, when invoked on bb, 
would have Buffer return type (the erasure), but that's not a problem - 
it is source compatible. To achieve the desired effect one would have to 
use at least:

     ByteBuffer<?> bb = ...;




I'm suggesting this alternative, because Buffer methods can stay final 
in this case. This is more JIT-friendly. And, if I'm not mistaken, 
client code compiled using JDK8 onto which this API change was 
backported, would still run with JDK8 (or JDK7 when compiled with 
-target 1.7) onto which the API change was not back-ported.


Regards, Peter


> . It also touches
> a couple of other classes which have unnecessary casts in. Aside from being
> a good idea, fixing the casts is also necessary due to -Werror.  Let me
> know if I've missed anything or if there are any outstanding issues.
>
> I think the bugs that this resolves are:
>
> https://bugs.openjdk.java.net/browse/JDK-4774077
> https://bugs.openjdk.java.net/browse/JDK-6451131
>
> regards,
>
>    Richard Warburton
>
>    http://insightfullogic.com
>    @RichardWarburto <http://twitter.com/richardwarburto>