[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #473: Implement variable direction property

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

mbeckerle commented on a change in pull request #473:
URL: https://github.com/apache/incubator-daffodil/pull/473#discussion_r562027522



##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -77,30 +76,52 @@ final class SetVariableUnparser(
 
 }
 
+final class NewVariableInstanceSuspendableExpression(
+  override val expr: CompiledExpression[AnyRef],
+  override val rd: VariableRuntimeData)
+  extends SuspendableExpression {
+
+  override protected def processExpressionResult(ustate: UState, v: DataValuePrimitive): Unit = {
+    ustate.variableMap.newVariableInstance(rd, v)
+  }
+
+  override protected def maybeKnownLengthInBits(ustate: UState) = MaybeULong(0)
+}
+

Review comment:
       The right behavior  is that suppose a DFDL implementation did not do streaming unparsing at all. There would be no-suspending when reaching forward for element values. 
   
   The behavior one gets in that situation is the correct answer.  Streaming unparsing has to achieve this same behavior, or it is a bug. 
   
   The correct behavior in the above example in steve lawrence's comment  is a runtime SDE due to setVariable variable1 occuring after the variable1 has already been read for its default value. 
    
   In Daffodil's streaming implementation, the forward referencing expression causing a suspension should make a copy/clone of all variable state, so such time as it does read variable1, it will get the default value of 1. 
   
   So far so good.
   
   As unparsing continues, the setVariable statement will be encountered, and it will set variable1's value to 2.  It does not know that the expression has suspended but will eventually read variable1.  This is the bug. 
   
   A new state on variable1 indicating it is pending a read by a suspension won't help. This doesn't work in general, because expressions can have if-then-else logic such that some conditional branches evaluate certain variables, not others. So until we evaluate an expression we can't know what variables it would read. 
   
   I believe we have a situation close to a classic compiler anti-dependence here. 
   
   Fact is, this setVariable should be itself suspended until after the variable-read in the expression for variable2. 
   
   The current daffodil expression implementation won't even attempt to read variable1's value until the suspension completes; hence, variable1 wouldn't even get marked as 'pending read' state until the forward reference has been resolved. 




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