[GitHub] [datasketches-memory] leerho commented on a change in pull request #139: Major refactor and fix

[GitBox <gi...@apache.org>]

leerho commented on a change in pull request #139:
URL: https://github.com/apache/datasketches-memory/pull/139#discussion_r689664744



##########
File path: datasketches-memory-java8/src/main/java/org/apache/datasketches/memory/WritableBuffer.java
##########
@@ -22,41 +22,47 @@
 
 import java.nio.ByteBuffer;
 import java.nio.ByteOrder;
+import java.util.Objects;
 
-import org.apache.datasketches.memory.internal.WritableBufferImpl;
+import org.apache.datasketches.memory.internal.BaseWritableBufferImpl;
+import org.apache.datasketches.memory.internal.Util;
 
 public interface WritableBuffer extends Buffer {
 
   //BYTE BUFFER
   /**
-   * Accesses the given ByteBuffer for write operations. The returned WritableBuffer object has
-   * the same byte order, as the given ByteBuffer, unless the capacity of the given ByteBuffer is
-   * zero, then byte order of the returned WritableBuffer object, as well as backing storage and
-   * read-only status are unspecified.
-   * @param byteBuf the given ByteBuffer, must not be null.
-   * @return a new WritableBuffer for write operations on the given ByteBuffer.
+   * Accesses the given <i>ByteBuffer</i> for write operations. The returned <i>WritableBuffer</i> object has
+   * the same byte order, as the given <i>ByteBuffer</i>.
+   * @param byteBuf the given ByteBuffer. It must be non-null and with capacity >= 0.
+   * @return a new <i>WritableBuffer</i> for write operations on the given <i>ByteBuffer</i>.
    */
   static WritableBuffer writableWrap(ByteBuffer byteBuf) {
-    return WritableBufferImpl.writableWrap(byteBuf);
+    return writableWrap(byteBuf, byteBuf.order(), defaultMemReqSvr);
   }
 
   /**
-   * Accesses the given ByteBuffer for write operations. The returned WritableBuffer object has
-   * the given byte order, ignoring the byte order of the given ByteBuffer. If the capacity of
-   * the given ByteBuffer is zero the byte order of the returned WritableBuffer object
-   * (as well as backing storage) is unspecified.
-   * @param byteBuf the given ByteBuffer, must not be null
-   * @param byteOrder the byte order to be used, which may be independent of the byte order
-   * state of the given ByteBuffer
-   * @param memReqSvr A user-specified MemoryRequestServer.
-   * This is a callback mechanism for a user client to request a larger Memory.
-   * @return a new WritableBuffer for write operations on the given ByteBuffer.
+   * Accesses the given <i>ByteBuffer</i> for write operations. The returned <i>WritableBuffer</i> object has
+   * the given byte order, ignoring the byte order of the given <i>ByteBuffer</i> for future writes and following reads.
+   * However, this does not change the byte order of data already in the <i>ByteBuffer</i>.
+   * @param byteBuf the given ByteBuffer. It must be non-null and with capacity >= 0.
+   * @param byteOrder the byte order to be used.
+   * @param memReqSvr A user-specified <i>MemoryRequestServer</i>, which must not be null.
+   * This is a callback mechanism for a user client to request a larger <i>WritableBuffer</i>.
+   * @return a new <i>WritableBuffer</i> for write operations on the given <i>ByteBuffer</i>.
    */
   static WritableBuffer writableWrap(ByteBuffer byteBuf, ByteOrder byteOrder, MemoryRequestServer memReqSvr) {
-    MemoryRequestServer mReqSvr = (memReqSvr == null) ? defaultMemReqSvr : memReqSvr;
-    return WritableBufferImpl.writableWrap(byteBuf, byteOrder, mReqSvr);
+    Objects.requireNonNull(byteBuf, "ByteBuffer 'byteBuf' must not be null");
+    Objects.requireNonNull(byteOrder, "ByteOrder 'byteOrder' must not be null");
+    Util.negativeCheck(byteBuf.capacity(), "byteBuf.capacity");
+    if (byteBuf.isReadOnly()) {
+      throw new ReadOnlyException("Cannot create a WritableBuffer from a ReadOnly ByteBuffer.");
+    }
+    return BaseWritableBufferImpl.wrapByteBuffer(byteBuf, false, byteOrder, memReqSvr);
   }

Review comment:
       Wherever the documentation says that, it needs to be changed.  We had a long discussion about whether the Default memReqSvr should even be there at all.   As it is now the DefaultMRSvr is set to null.  This forces the user to specifically implement what they want the MRSvr to do.  The default serves as an example. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org