[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #972: Update occursIndex to skip absent elements

["stevedlawrence (via GitHub)" <gi...@apache.org>]

stevedlawrence commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1117288944


##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/DPathRuntime.scala:
##########
@@ -64,6 +64,7 @@ class CompiledDPath(val ops: RecipeOp*) extends Serializable {
     }
 
     dstate.setArrayPos(state.arrayPos)

Review Comment:
   Can we remove `arrayPos` from the DState? I don't think anything in expressions needs that information anymore. It only ever needs the occursPos.



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/PState.scala:
##########
@@ -108,12 +113,16 @@ object MPState {
 class MPState private () {
 
   val arrayIndexStack = MStackOfLong()
+  val occursIndexStack = MStackOfLong()
 
   def moveOverOneArrayIndexOnly() = arrayIndexStack.push(arrayIndexStack.pop + 1)
   def moveBackOneArrayIndexOnly() = arrayIndexStack.push(arrayIndexStack.pop - 1)
 
   def arrayPos = arrayIndexStack.top
 
+  def incrementOccursIndex() = occursIndexStack.push(occursIndexStack.pop + 1)

Review Comment:
   For consistency, I'd suggest naming this `moveOverOneOccursIndexOnly`. 
   
   Why don't we need a `moveBackOnOccursIndexOnly`? I would imagine in cases where we decrement aray index we might also need to decrement occurs index?
   
   Actually, it looks like nothing calls moveBackOneArrayIndex, so maybe we should just remove that.



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/PState.scala:
##########
@@ -108,12 +113,16 @@ object MPState {
 class MPState private () {
 
   val arrayIndexStack = MStackOfLong()

Review Comment:
   Is it worth renaming this to something like `arrayIterationIndexStack` to make it more clear that it is the array iteration count and not the actual index of the current array element, since the two aren't necessarily the same. 
   
   I also wonder if we want to do the same with `arrayPos` and rename it to `arrayIterationPos`? I think `arrayPos` is even more confusing as a name, since it really does make me think it's the position in the array, which is isn't always the case.
   
   I think this PR would be a good time to look at places where `arrayPos` is used and make sure it's used correctly, since it's possible `dfdl:occursIndex` isn't the only case.



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/ProcessorStateBases.scala:
##########
@@ -507,6 +507,7 @@ abstract class ParseOrUnparseState protected (
 
   def groupPos: Long
   def arrayPos: Long
+  def occursIndex: Long

Review Comment:
   Should this be occursPos to be consistent with other naming conventions?



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/unparsers/UState.scala:
##########
@@ -459,7 +464,8 @@ final class UStateForSuspension(
 
   override def groupPos = 0 // was die, but this is called when copying state during debugging
   override def currentInfosetNodeMaybe = Maybe(currentInfosetNode)
-  override def arrayPos = occursIndex
+  override def arrayPos = arrayIndex
+  override def occursIndex = occurs

Review Comment:
   Suggest occursPos for consisence.



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/unparsers/UState.scala:
##########
@@ -413,7 +415,8 @@ final class UStateForSuspension(
   override var dataOutputStream: DirectOrBufferedDataOutputStream,
   vbox: VariableBox,
   override val currentInfosetNode: DINode,
-  occursIndex: Long,
+  arrayIndex: Long,
+  occurs: Long,

Review Comment:
   Suggest `occursIndex` for consistency.



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceParserBases.scala:
##########
@@ -194,6 +194,12 @@ abstract class SequenceParserBase(
                 pstate.mpstate.moveOverOneArrayIndexOnly()
               }
 
+              // If the emptyElementParsePolicy is set to treatAsAbsent, don't
+              // increment the occursIndex if the element is absent
+              if (resultOfTry != AbsentRep ) {
+                pstate.mpstate.incrementOccursIndex()
+              }

Review Comment:
   This is the only difference between arrayIndex and occursIndex, right? Why do we increment occursIndex when ais eq Done? We exclude that for arrayIndex for some reason?



-- 
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@daffodil.apache.org

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