stevedlawrence commented on a change in pull request #473:
URL: https://github.com/apache/incubator-daffodil/pull/473#discussion_r562004598
##########
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:
One thing to that jumped to my mind, does a setVariable even need to
suspend if the defaultValue is suspending (i.e. the vairables is in the new
VariableSuspended state)? In this case, we'll never use the defaultValue, so
why wait for it to become evaluated if setVariable doesn't need to suspend?
I think the answer here is the defaultValue could access other variables
which changes the state of those variables to read, so it probably is important
to wait for the defaultValue to evaluate, even if it's value will be overwriten
by a setVariable.
Another unrelated thought about how this all needs to work, say we had
something like this:
* NVI variable1, defaultValue = 1
* NVI variable2, defaultValue = "{
/some/future/path/that/requires/suspension + $variable1 }"
* setVariable, variable1 = 2
* element A dfdl:inputValueCalc="{ $variable1 }"
So we create NVI variable1 with an immediate value of defaultValue1. All
good.
Then we create a NVI variable2, but the defaultValue must suspend because it
accesses some future path. In suspending, it clones the UState (including the
current "1" value of variable1). Note that it hasn't read variable1 until the
path comes into existance.
We continue the unparse and evaluate the setVariable and change variable1 to
a value of 2. This is fine because variable1 hasn't been read yet, because NVI
variable2 defaultValue suspended.
Continuing the parse, we evaluate the inputValueCalc and set element A to
variable1, which now has a value of 2.
At some point we unsuspend the defaultValue, and set defalutValue to 1. I
*think* this works because this suspension is working on a cloned UState. But
this uses the 1 value from variable1 because it was cloned. This feels wrong?
And we're done, and it's not clear what the right behavior was.
Note that if variable2 didn' need to suspend (say that long path existed),
then variable1 is immedately read when setting variable2's defaultValue, and so
the setVariable for variable1 should be an SDE.
So it seems we could get two different behaviors depending if defaultValue
suspends or not, and it's not immediately clear to me what the right behavior
is. Hopefully I'm missing something?
##########
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:
Thinking more on this, I think there's maybe some other complexities
related to variables, and doesn't even include NVI.
Imagine we having a variable with a constant static default, and then a
setVariable to set that variable but that must suspend due to a forward
lookahead. Now if an an element tries to read that variable, it must also
suspend until that setVariable completes. Something to consider with this is
that setVariable suspension is working on a cloned ustate. So the cloned ustate
must do a shallow copy of the variable map so that when it does modify the
variable map, other suspensions will see that change. I think this may be how
it works, but worth having some tests to consider.
But also note that since this is a shallow copy, it means other
suspensions/parsers could modify this variable map, and could maybe potentially
affect the suspension once it unsuspends? Maybe something like this:
* variable1 defaultValue = 1
* variable2 defaultValue = 2
* setVariable variable1, value = /forward/path/forcing/suspension + variable2
* setVariable variable2, value = 3
So setVariable variable1 must suspend. Then setVariable varible2 changes the
value of variable2 to 3. Then at some point when setVariable variable1
unsuspends, it gets the value of 3 for variable2 (assuming shallow variable map
copy in the ustate clone). The correct result is setVariable2 should cause an
SDE because setVariable variable1 should have read variable2. So this too has
this order dependency stuff going on and doesn't involve NVI. I think this is
basically the same thing as my previous scenario, but doesn't use NVI.
----------------------------------------------------------------
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:
[email protected]
