Suggestions from PR #1022

amazon-ion · Jan 7, 2025 · cad1078 · cad1078
1 parent d265972
commit cad1078
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 48 deletions.
diff --git a/src/main/java/com/amazon/ion/impl/IonCursorBinary.java b/src/main/java/com/amazon/ion/impl/IonCursorBinary.java
@@ -1994,59 +1994,40 @@ boolean uncheckedSkipRemainingDelimitedContainerElements_1_1() {
      */
     private boolean slowSkipMacroParameter(Macro.Parameter parameter) {
         switch (parameter.getType()) {
-            case Tagged:
-                slowNextToken();
-                if (event == Event.NEEDS_DATA) {
-                    return true;
-                }
-                break;
             case Int8:
             case Uint8:
-                if (!fillAt(peekIndex, 1)) {
-                    return true;
-                }
-                peekIndex += 1;
-                break;
+                return fillAndUpdatePeekIndex(1);
             case Int16:
             case Uint16:
             case Float16:
-                if (!fillAt(peekIndex, 2)) {
-                    return true;
-                }
-                peekIndex += 2;
-                break;
+                return fillAndUpdatePeekIndex(2);
             case Int32:
             case Uint32:
             case Float32:
-                if (!fillAt(peekIndex, 4)) {
-                    return true;
-                }
-                peekIndex += 4;
-                break;
+                return fillAndUpdatePeekIndex(4);
             case Int64:
             case Uint64:
             case Float64:
-                if (!fillAt(peekIndex, 8)) {
-                    return true;
-                }
-                peekIndex += 8;
-                break;
+                return fillAndUpdatePeekIndex(8);
             case FlexUint:
-                if (slowReadFlexUInt_1_1() < 0) {
-                    return true;
-                }
-                break;
+                return slowReadFlexUInt_1_1() < 0;
             case FlexInt:
-                if (slowReadFlexInt_1_1(valueMarker)) {
-                    return true;
-                }
-                break;
+                return slowReadFlexInt_1_1(valueMarker);
             case FlexSym:
-                if (FlexSymType.INCOMPLETE == slowSkipFlexSym_1_1(valueMarker)) {
-                    return true;
-                }
-                break;
+                return FlexSymType.INCOMPLETE == slowSkipFlexSym_1_1(valueMarker);
+            case Tagged:
+                slowNextToken();
+                return event == Event.NEEDS_DATA;
+            default:
+                return false;
         }
+    }
+
+    private boolean fillAndUpdatePeekIndex(int numberOfBytes) {
+        if (!fillAt(peekIndex, numberOfBytes)) {
+            return true;
+        }
+        peekIndex += numberOfBytes;
         return false;
     }
 
@@ -3001,14 +2982,12 @@ Event stepOutOfEExpression() {
         if (!parent.typeId.isMacroInvocation) {
             throw new IonException("Not positioned within an e-expression.");
         }
-        if (isSlowMode) {
-            if (slowSeekToEndOfEExpression()) {
-                return event;
-            }
-            slowPopContainer();
-        } else {
+        if (!isSlowMode) {
             uncheckedSeekToEndOfEExpression();
             uncheckedPopContainer();
+        } else if (!slowSeekToEndOfEExpression()) {
+            // we had enough data to seek to the end of the expression
+            slowPopContainer();
         }
         return event;
     }
@@ -3075,7 +3054,7 @@ private void slowPopContainer() {
         setCheckpointBeforeUnannotatedTypeId();
         if (--containerIndex >= 0) {
             parent = containerStack[containerIndex];
-        } else {
+        } else { // we're at top level
             parent = null;
             containerIndex = -1;
         }

diff --git a/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java b/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java
@@ -6350,7 +6350,8 @@ public void traverseDelimitedStructThatContainsMacroInvocationWithTaggedExpressi
     public void fillNestedDelimitedLists() throws Exception {
         readerBuilder = readerBuilder.withBufferConfiguration(
             IonBufferConfiguration.Builder.standard()
-                .withInitialBufferSize(6) // This ensures the entire delimited container is not fully buffered up front.
+                // Buffer cuts off before we've seen all the start-container bytes
+                .withInitialBufferSize(6)
                 .onData(byteCountingHandler)
                 .build()
         );
@@ -6381,7 +6382,8 @@ public void fillNestedDelimitedLists() throws Exception {
     public void fillNestedEmptyDelimitedList() throws Exception {
         readerBuilder = readerBuilder.withBufferConfiguration(
             IonBufferConfiguration.Builder.standard()
-                .withInitialBufferSize(7) // This ensures the entire delimited container is not fully buffered up front.
+                // Buffer cuts off before we've seen all the end-container bytes
+                .withInitialBufferSize(7)
                 .onData(byteCountingHandler)
                 .build()
         );
@@ -6403,7 +6405,8 @@ public void fillNestedEmptyDelimitedList() throws Exception {
     public void fillMultipleNestedDelimitedLists() throws Exception {
         readerBuilder = readerBuilder.withBufferConfiguration(
             IonBufferConfiguration.Builder.standard()
-                .withInitialBufferSize(7) // This ensures the entire delimited container is not fully buffered up front.
+                // Buffer cuts off before we've seen all the start-container bytes
+                .withInitialBufferSize(7)
                 .onData(byteCountingHandler)
                 .build()
         );