[GitHub] [lucene] dweiss commented on a change in pull request #348: LUCENE-10143: Make DataInput/Output short/int/long reads and writes abstract

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

dweiss commented on a change in pull request #348:
URL: https://github.com/apache/lucene/pull/348#discussion_r720782981



##########
File path: lucene/core/src/java/org/apache/lucene/store/ByteBuffersDataInput.java
##########
@@ -103,6 +103,24 @@ public byte readByte() throws EOFException {
     }
   }
 
+  @Override
+  public short readShort() throws IOException {
+    // TODO: use ByteBuffer.getShort

Review comment:
       + add an assertion somewhere ensuring proper byte order on all blocks?

##########
File path: lucene/core/src/java/org/apache/lucene/codecs/MultiLevelSkipListReader.java
##########
@@ -319,6 +320,27 @@ public void readBytes(byte[] b, int offset, int len) {
       pos += len;
     }
 
+    @Override
+    public short readShort() throws IOException {
+      short value = (short) BitUtil.VH_LE_SHORT.get(pos);
+      pos += Short.BYTES;
+      return value;
+    }
+
+    @Override
+    public int readInt() throws IOException {
+      int value = (int) BitUtil.VH_LE_INT.get(pos);
+      pos += Integer.BYTES;
+      return value;
+    }
+
+    @Override
+    public long readLong() throws IOException {

Review comment:
       All multibyte-reading implementations with position increment after the read suffer from the same subtle issue: imagine you're at file length-3 bytes offset and trying to read an int: readInt() would fail but a subsequent readByte() or readShort() would succeed. I'm not sure if we need to make it a contract that the bytes are always consumed by readXYZ but if we don't then it'd be good to document this somewhere (even as an "unspecified behavior on insufficient bytes").




-- 
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: issues-unsubscribe@lucene.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org