[GitHub] [datasketches-memory] davecromberge commented on a change in pull request #129: Create internal2

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

davecromberge commented on a change in pull request #129:
URL: https://github.com/apache/datasketches-memory/pull/129#discussion_r636862050



##########
File path: src/main/java/org/apache/datasketches/memory/BaseBuffer.java
##########
@@ -143,11 +92,7 @@ public final BaseBuffer setPosition(final long position) {
    * @param position the given current position.
    * @return BaseBuffer
    */
-  public final BaseBuffer setAndCheckPosition(final long position) {
-    checkInvariants(start, position, end, capacity);
-    pos = position;
-    return this;
-  }
+  BaseBuffer setAndCheckPosition(long position);

Review comment:
       Do these variants of the interface that check arguments imply that the client typically would expect and handle a possible `IllegalArgumentException`?  How does this differ in severity from an `AssertionError`?

##########
File path: src/main/java/org/apache/datasketches/memory/internal/BaseBufferImpl.java
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.datasketches.memory.internal;
+
+import org.apache.datasketches.memory.BaseBuffer;
+
+/**
+ * A new positional API. This is different from and simpler than Java BufferImpl positional approach.
+ * <ul><li>All based on longs instead of ints.</li>
+ * <li>Eliminated "mark". Rarely used and confusing with its silent side effects.</li>
+ * <li>The invariants are {@code 0 <= start <= position <= end <= capacity}.</li>
+ * <li>It always starts up as (0, 0, capacity, capacity).</li>
+ * <li>You set (start, position, end) in one call with
+ * {@link #setStartPositionEnd(long, long, long)}</li>
+ * <li>Position can be set directly or indirectly when using the positional get/put methods.
+ * <li>Added incrementPosition(long), which is much easier when you know the increment.</li>
+ * <li>This approach eliminated a number of methods and checks, and has no unseen side effects,
+ * e.g., mark being invalidated.</li>
+ * <li>Clearer method naming (IMHO).</li>
+ * </ul>
+ *
+ * @author Lee Rhodes
+ */
+public abstract class BaseBufferImpl extends BaseStateImpl implements BaseBuffer {
+  private long capacity;
+  private long start = 0;
+  private long pos = 0;
+  private long end;
+
+  //Pass-through ctor
+  BaseBufferImpl(final Object unsafeObj, final long nativeBaseOffset,
+      final long regionOffset, final long capacityBytes) {
+    super(unsafeObj, nativeBaseOffset, regionOffset, capacityBytes);
+    capacity = end = capacityBytes;
+  }
+
+  @Override
+  public final BaseBufferImpl incrementPosition(final long increment) {
+    incrementAndAssertPositionForRead(pos, increment);
+    return this;
+  }
+
+  @Override
+  public final BaseBufferImpl incrementAndCheckPosition(final long increment) {
+    incrementAndCheckPositionForRead(pos, increment);
+    return this;
+  }
+
+  @Override
+  public final long getEnd() {
+    return end;
+  }
+
+  @Override
+  public final long getPosition() {
+    return pos;
+  }
+
+  @Override
+  public final long getStart() {
+    return start;
+  }
+
+  @Override
+  public final long getRemaining()  {
+    return end - pos;
+  }
+
+  @Override
+  public final boolean hasRemaining() {
+    return (end - pos) > 0;
+  }
+
+  @Override
+  public final BaseBufferImpl resetPosition() {
+    pos = start;
+    return this;
+  }
+
+  @Override
+  public final BaseBufferImpl setPosition(final long position) {
+    assertInvariants(start, position, end, capacity);
+    pos = position;
+    return this;
+  }
+
+  @Override
+  public final BaseBufferImpl setAndCheckPosition(final long position) {
+    checkInvariants(start, position, end, capacity);
+    pos = position;
+    return this;
+  }
+
+  @Override
+  public final BaseBufferImpl setStartPositionEnd(final long start, final long position,
+      final long end) {
+    assertInvariants(start, position, end, capacity);
+    this.start = start;
+    this.end = end;
+    pos = position;
+    return this;
+  }
+
+  @Override
+  public final BaseBufferImpl setAndCheckStartPositionEnd(final long start, final long position,
+      final long end) {
+    checkInvariants(start, position, end, capacity);
+    this.start = start;
+    this.end = end;
+    pos = position;
+    return this;
+  }
+
+  //RESTRICTED
+  final void incrementAndAssertPositionForRead(final long position, final long increment) {
+    assertValid();
+    final long newPos = position + increment;
+    assertInvariants(start, newPos, end, capacity);
+    pos = newPos;
+  }
+
+  final void incrementAndAssertPositionForWrite(final long position, final long increment) {
+    assertValid();
+    assert !isReadOnly() : "BufferImpl is read-only.";
+    final long newPos = position + increment;
+    assertInvariants(start, newPos, end, capacity);
+    pos = newPos;
+  }
+
+  final void incrementAndCheckPositionForRead(final long position, final long increment) {
+    checkValid();
+    final long newPos = position + increment;
+    checkInvariants(start, newPos, end, capacity);
+    pos = newPos;
+  }
+
+  final void incrementAndCheckPositionForWrite(final long position, final long increment) {
+    checkValidForWrite();
+    final long newPos = position + increment;
+    checkInvariants(start, newPos, end, capacity);
+    pos = newPos;
+  }
+
+  final void checkValidForWrite() {
+    checkValid();
+    if (isReadOnly()) {
+      throw new ReadOnlyException("BufferImpl is read-only.");
+    }
+  }

Review comment:
       These restricted methods are used by subclasses, should they be marked as `protected`?




-- 
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.

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