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

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

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



##########
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:
       There are two types of checks: "check(blah)" and "assert(blah)".   Asserts only apply when running test or if the JVM flag -ea is set.  The check methods will throw an exception.  Note that checks are primarily used when copying arrays or blocks. but when just setting primitives, I use asserts.  The primary reason is speed during runtime, otherwise, the cost of the check would overwhelm the actual operation.  If the user is concerned about memory violations, all they have to do is set -ea.    

##########
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:
       When subclasses are in the same package, the subclasses can access via package-private.  Keeping these package-private prevents users from attempting to subclass from another package.  Thus, package-private is more restrictive than 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