You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2021/03/15 20:13:00 UTC
[GitHub] [daffodil] jadams-tresys opened a new pull request #510: New variable instances should inherit external values
jadams-tresys opened a new pull request #510:
URL: https://github.com/apache/daffodil/pull/510
There was a bug where variables that were given external values were not
having those values passed on by default to newVariableInstance's. This
commit addresses this issue by setting NVI's to the externally provided
value by default if no other default value is specified for the NVI.
DAFFODIL-2481
----------------------------------------------------------------
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r596286553
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -104,7 +104,10 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override def unparse(state: UState) = {
state.variableMap.newVariableInstance(context)
- if (context.maybeDefaultValueExpr.isDefined) {
+
+ // Only process the default value expression if it is defined and only if
+ // the variable is non-external or has a local default
+ if (context.maybeDefaultValueExpr.isDefined && (!context.external || context.hasLocalDefault)) {
Review comment:
Seems a reasonable argument to me.
So in that case, a decent amount of this changes. I think we no longer need the isLocalDefault because the existance of maybeDefaultValueExpr gives us that. And we now need to capture the original defineVariable value (whether it be set externally or via a defaultValue expression) and make that available to NVIs for use when then maybeDefaultValueExpr isn't set.
----------------------------------------------------------------
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r601777897
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -103,17 +103,23 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.variableMap.newVariableInstance(context)
-
- // Only process the default value expression if it is defined and only if
- // the variable is non-external or has a local default
- if (context.maybeDefaultValueExpr.isDefined && (!context.external || context.hasLocalDefault)) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ val orig = state.variableMap.getFirstInstance(context.globalQName).get
+ state.variableMap.newVariableInstance(context, orig.defaultValue)
+
+ val v = state.variableMap.find(context.globalQName).getOrElse(Assert.invariantFailed("Unable to find variable %s".format(context.globalQName)))
+
+ // If the NVI has a default value expression, process it to replace the
+ // default value inherited from the global defineVariable statement.
+ if (context.maybeDefaultValueExpr.isDefined) {
+ v.setState(VariableInProcess)
val suspendableExpression = new NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get, context)
suspendableExpression.run(state)
}
+
+ if (v.value != DataValue.NoValue)
+ v.setState(VariableDefined)
Review comment:
You are right, the state should have been set when we call setDefaultValue on the variable instance. Will make the change.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602484294
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -103,6 +104,9 @@ class VariableInstance private (
lazy val defaultValueExpr = defaultValueExprArg
+ // This represents the default value at the start of processing, provided by
Review comment:
Yeah, I think you're right that there are places where we only have access to the instance.
That said, I think anything that can simplify this variable stuff is worth the effort. It's taken me multiple reviews to get an idea of how it all works, and I still don't understand it all (that isn't saying anything about yoru code, this stuff is just complicated).
For example, I noticed that each ``VariableInstance`` already has a pointer to its VariableRuntimeData, so could we remove these defaultValueExpr/Arg members and access the expression on the RuntimeData when needed?
Also, I noticed in the VRD that it stores state and value depending on if the if there's a defaultValue expression, and then that is used to initialize the VariableInstance. This feels unnecessary and wrong to me. We shouldnt' be creating a VariableInstance with some state based on whether or not it has a defaultValue, since things like external values also play a role. It seems like we should always create an empty VariableInstance with no value/state, and the all the new you've added should be setting default value/state as appropriate. In fact, my guess is this is where the value=NoValue but state=VariableDefined comes from.
All that said, I wonder if instead, a VariableInstance should take no parameters except for VariableRuntimeData. So the value, state, priorValue, priorState, and defaultValue become member variables. When we create a VariableInstance, it's value is initialize to None and state to VariableUndefined. Then all the logic you've added will get triggered during parse to figure out default values and set state appropriately.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r601780855
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -148,7 +154,11 @@ class VariableInstance private (
this._value = Some(v)
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
+ /* This is used to set a default value without assigning a priorValue */
Review comment:
You're right, forceExpressionEvaluations should be using setDefaultValue.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r596277724
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -104,7 +104,10 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override def unparse(state: UState) = {
state.variableMap.newVariableInstance(context)
- if (context.maybeDefaultValueExpr.isDefined) {
+
+ // Only process the default value expression if it is defined and only if
+ // the variable is non-external or has a local default
+ if (context.maybeDefaultValueExpr.isDefined && (!context.external || context.hasLocalDefault)) {
Review comment:
I think given an NVI without a default value, and a defineVariable with an expression for the default value, the assumption should be that the defineVariable default value is computed once, at start of processing, and that value is reused. The expression would not be re-evaluated when the NVI instance is created.
Suppose your var2 was also external="true", so that the default value can be overridden externally. In that case the default value expression would never be evaluated.
There is no place in DFDL where "inherit the expression" semantics is used I think.
E.g., suppose you use a property from a dfdl:format default format and there is an expression used there. That expression is evaluated once only. Not every time that property is referenced. If that expression references variables then the values of those variables are only the global values of them.
The "inherit the expression" semantics can be obtained by creating new variable instances of both your var1 and var2 where var2 has its own default value expression of "{ $var1 }".
`<dfdl:newVariableInstance ref="var1" />
<dfd:setVariable ref="var1" value="Y" />
<dfdl:newVariableInstance ref="var2" defaultValue="{ $var1 }"/>`
Here var2's default value would be Y.
----------------------------------------------------------------
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r595222228
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -953,6 +953,7 @@ final class VariableRuntimeData(
pathArg: String,
namespacesArg: NamespaceBinding,
val external: Boolean,
+ val hasLocalDefault: Boolean,
val direction: VariableDirection,
@TransientParam maybeDefaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
Review comment:
You interpreted the situation correctly. maybeDefaultValueExprArg will inherit from the variable definition if there isn't one local to the newVariableInstance. I can make the name change to try to clear this up a little more.
----------------------------------------------------------------
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r607841906
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -103,13 +102,22 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.variableMap.newVariableInstance(context)
+ val v = state.variableMap.newVariableInstance(context)
+
if (context.maybeDefaultValueExpr.isDefined) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
- val suspendableExpression = new NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get, context)
- suspendableExpression.run(state)
+ val dve = context.maybeDefaultValueExpr.get
+ dve match {
+ case constExpr: ConstantExpression[_] => // Do nothing, value has already been computed at schema compile time
+ case _ => {
+ v.setState(VariableInProcess)
+ val suspendableExpression = new NewVariableInstanceDefaultValueSuspendableExpression(dve, context)
Review comment:
Can we pass in the variable instance instance the NewVariableInstanceDefaultValue.... so that we don't have to do a variableMap.find in processExpressionResult?
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -103,13 +102,22 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.variableMap.newVariableInstance(context)
+ val v = state.variableMap.newVariableInstance(context)
+
if (context.maybeDefaultValueExpr.isDefined) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
- val suspendableExpression = new NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get, context)
- suspendableExpression.run(state)
+ val dve = context.maybeDefaultValueExpr.get
+ dve match {
+ case constExpr: ConstantExpression[_] => // Do nothing, value has already been computed at schema compile time
+ case _ => {
+ v.setState(VariableInProcess)
+ val suspendableExpression = new NewVariableInstanceDefaultValueSuspendableExpression(dve, context)
+ suspendableExpression.run(state)
+ }
+ }
+ } else {
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ v.setDefaultValue(v.firstInstanceInitialValue)
Review comment:
Just had a though, what if a variable did not have a default value and was not set externaly? Then firstInstanceInitialValue is NoValue, and so we are going to set the default value to not value. Which changes the state to Defined. So we could get in a state where we are Defined but with NoValue. Do we want to make it so firstInstanceInitialValue is an Option, and this becomes something like:
```scala
} else if (v.firstInstanceInitialValue.isDefined) {
v.setDefaultValue(v.firstInstanceInitialValue.get)
}
```
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -148,22 +168,26 @@ class VariableInstance private (
this._value = Some(v)
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
+ /* This is used to set a default value without assigning a priorValue */
+ def setDefaultValue(v: DataValuePrimitiveNullable) = {
+ Assert.invariant(this.state == VariableUndefined || this.state == VariableInProcess)
Review comment:
Should we assert the ``v != NoValue`` too?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -324,17 +348,31 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
// have default value expressions or are defined externally.
def forceExpressionEvaluations(state: ParseOrUnparseState): Unit = {
vTable.foreach { case (_, variableInstances) => { variableInstances.foreach { inst => {
- (inst.state, inst.value, inst.defaultValueExpr.isDefined) match {
+ (inst.state, inst.defaultValueExpr.isDefined) match {
// Evaluate defineVariable statements with non-constant default value expressions
- case (VariableDefined, DataValue.NoValue, true) => {
+ case (VariableUndefined, true) => {
val res = inst.defaultValueExpr.get.evaluate(state)
- inst.setValue(DataValue.unsafeFromAnyRef(res))
+ inst.setDefaultValue(DataValue.unsafeFromAnyRef(res))
}
- case (_, _, _) => // Do nothing
+ case (_, _) => // Do nothing
}
}}}}
}
+
+ /**
+ * This function is called immediately after forceExpressionEvaluations in
+ * order to set the firstInstanceInitialValue for each variable instance.
+ * These initial values will be inherited by future new variable instances if
+ * the newVariableInstance statement does not provide a default value
+ * expression
+ */
+ def setFirstInstanceInitialValues(): Unit = {
+ vTable.foreach { case (_, variableInstances) => {
+ variableInstances.foreach { inst => inst.firstInstanceInitialValue = inst.value }
Review comment:
variableInstances should always have a length of 1 here, right? Since this should only ever be called before NVI's are evaluated? Thoughts on making this
```scala
Assert.invariant(variableInstances.length == 1)
variablesInstances(0).firstInstanceInitialValue = inst._value
```
Note that if firstInstanceInitialValue is turned into an Option mentioend above, then this is just a straigt copy of the private _value. So it keeps the Some/None aspect of the value as well if a instance was never set by a default.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -978,17 +978,12 @@ final class VariableRuntimeData(
@throws(classOf[java.io.IOException])
final private def writeObject(out: java.io.ObjectOutputStream): Unit = serializeObject(out)
- private lazy val state =
- if (!maybeDefaultValueExpr.isDefined) VariableUndefined
- else VariableDefined
-
- private lazy val value: DataValuePrimitiveNullable =
- if (maybeDefaultValueExpr.isEmpty) DataValue.NoValue
- else {
- val defaultValueExpr = maybeDefaultValueExpr.get
- defaultValueExpr match {
- case constExpr: ConstantExpression[_] => DataValue.unsafeFromAnyRef(constExpr.constant)
- case _ => DataValue.NoValue
+ private lazy val (state, value) = {
+ if (!maybeDefaultValueExpr.isDefined)
+ (VariableUndefined, DataValue.NoValue)
+ else maybeDefaultValueExpr.get match {
+ case constExpr: ConstantExpression[_] => (VariableDefined, DataValue.unsafeFromAnyRef(constExpr.constant))
+ case _ => (VariableUndefined, DataValue.NoValue)
Review comment:
Ah, so this is how you handle constants? I'm curious what others say, but I'd rather not have this special case here. It's maybe a little more performant when it is a constant, but it adds extra special cases and spreads out the NVI default logic in separate places. And I suspect NVI default values will rarely be constants, so we probably won't even hit this case very often.
I'd rather we just delete this state/value thing entirely, change the createVariableInstance function always creates a new instances with VariableUndefinedState and NoValue, and then the parsers/unparsers can mutate the state in a single place.
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -103,13 +102,22 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.variableMap.newVariableInstance(context)
+ val v = state.variableMap.newVariableInstance(context)
+
if (context.maybeDefaultValueExpr.isDefined) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
- val suspendableExpression = new NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get, context)
- suspendableExpression.run(state)
+ val dve = context.maybeDefaultValueExpr.get
+ dve match {
+ case constExpr: ConstantExpression[_] => // Do nothing, value has already been computed at schema compile time
Review comment:
Not sure I understand this. If the defaultValue for the NVI is constant, we still need to get that constant value and set it as the defalutValue, right? Seems like this should be something like
```scala
v.setDefalutValue(constExpr.evaluate(state))
```
Though, this is essentially what the NewVariableInstanceSuspendableExpression will do, so I'm not sure if we need a special case for constant. I guess it does avoid allocating this suspendable thing, but that's probably not too big of a deal? And since the expression is constant, it shouldn't even suspend, so we will avoid that overhead regardless.
Can we remove this special case and just always do the below?
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608562903
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,35 +82,26 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
+ state: VariableState = VariableUndefined,
+ value: DataValuePrimitiveNullable = DataValue.NoValue,
rd: VariableRuntimeData,
defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
Review comment:
Do we need to pass in defaultValueExprArg anymore? I think your recent changes made it so we always get the default expression from the VariableRuntimeData when it's need?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,35 +82,26 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
+ state: VariableState = VariableUndefined,
+ value: DataValuePrimitiveNullable = DataValue.NoValue,
rd: VariableRuntimeData,
defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
priorState: VariableState = VariableUndefined,
priorValue: DataValuePrimitiveNullable = DataValue.NoValue) = {
- new VariableInstance(stateArg, valueArg, rd, defaultValueExprArg, priorState, priorValue)
+ new VariableInstance(state, value, rd, defaultValueExprArg, priorState, priorValue)
}
}
/**
* See documentation for object VariableInstance
*/
class VariableInstance private (
- @TransientParam stateArg: => VariableState,
- @TransientParam valueArg: => DataValuePrimitiveNullable,
+ var state: VariableState,
+ var value: DataValuePrimitiveNullable,
val rd: VariableRuntimeData,
@TransientParam defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
Review comment:
Thoughts on moving state, value, priorState, and priorValue to be members of this class, rather than constructor parameters? I think everywhere we create a VariableInstance now these values are always VairableUndefined/NoValue, and with your changes I can't think of a reason why we would ever need to create one of these with something different?
If so, and if we can remove defaultValueExpr, maybe the only parameter is the VariableRunimeData?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/ExpressionEvaluatingParsers.scala
##########
@@ -165,11 +166,21 @@ class NewVariableInstanceStartParser(override val context: VariableRuntimeData)
override lazy val runtimeDependencies = Vector()
def parse(start: PState): Unit = {
- start.newVariableInstance(context)
+ val nvi = start.newVariableInstance(context)
+
if (context.maybeDefaultValueExpr.isDefined) {
- val v = start.variableMap.find(context.globalQName).get
- val res = DataValue.unsafeFromAnyRef(context.maybeDefaultValueExpr.get.evaluate(start))
- v.setDefaultValue(res)
+ val dve = context.maybeDefaultValueExpr.get
+ dve match {
+ case constExpr: ConstantExpression[_] => // Do nothing, value has already been computed at schema compile time
Review comment:
Remove the constant thing here so it matches unparse?
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r609036750
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,35 +82,26 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
+ state: VariableState = VariableUndefined,
+ value: DataValuePrimitiveNullable = DataValue.NoValue,
rd: VariableRuntimeData,
defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
Review comment:
Do we even need to do that? If something needs acces to it could it just do ``vrd.defaultValueExpr``, assume defaultValueExpr is made public if it's not already? Do we need a refrence in both the VariabeInstance and the VRD?
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608969862
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/ExpressionEvaluatingParsers.scala
##########
@@ -165,11 +166,21 @@ class NewVariableInstanceStartParser(override val context: VariableRuntimeData)
override lazy val runtimeDependencies = Vector()
def parse(start: PState): Unit = {
- start.newVariableInstance(context)
+ val nvi = start.newVariableInstance(context)
+
if (context.maybeDefaultValueExpr.isDefined) {
- val v = start.variableMap.find(context.globalQName).get
- val res = DataValue.unsafeFromAnyRef(context.maybeDefaultValueExpr.get.evaluate(start))
- v.setDefaultValue(res)
+ val dve = context.maybeDefaultValueExpr.get
+ dve match {
+ case constExpr: ConstantExpression[_] => // Do nothing, value has already been computed at schema compile time
Review comment:
Missed that, will fix.
--
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
[GitHub] [daffodil] jadams-tresys commented on pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on pull request #510:
URL: https://github.com/apache/daffodil/pull/510#issuecomment-814520592
Note that the last commit produces 3 failures in external variable unit
tests, ie non TDMLRunner tests. They rely on variables constant value
defaultValueExpressions to be evaluated at schema compile time. I'm
thinking that these tests were remnants of a time whne TDMLRunner didn't
support external variables and we simply remove these tests after making
sure we aren't losing any coverage by only using TDMLRunner tests.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602298239
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -105,6 +105,9 @@ class VariableInstance private (
lazy val defaultValueExpr = defaultValueExprArg
+ // This represents the default value at the start of processing, provided by
+ // either the defaultValue expression or by an external binding
+ var defaultValue: DataValuePrimitiveNullable = DataValue.NoValue
Review comment:
I agree, the copying of the ``fisrtInstanceInitialValue`` thing does seem a bit weird. What I was trying to avoid by suggesting this was a member variable that only has a value in the first instance. If we didn't do this copy and someone accidentally accessed this member from an instance other than the first one, then they'll probably get a null value or some unexpected behavior. By copying the value from the first instance to all following instances, it doesn't matter from which instance you access this value, you will always get the initial value of the first instance.
Maybe a way to avoid that would be do have another class, e.g.
```scala
class InitialVariableInstance extends VariableInstance {
var initialValue = ...
}
```
This way only the ``IntialVariableInstance`` has this initialValue variable, and so you can never accidentally access it from some other instance, because other instances don't even have this variable. I'm not sure if that's worth it though. You end up needing something like this to get the initial value:
```scala
instances(0).asInstanceOf[InitialVariableInstance].initialValue
```
I'm sure we could come up with additionl member variables/functions to get around this and make it look cleaner, but I'm not convinced it's entirely worth it for just one member.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602419401
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -103,6 +104,9 @@ class VariableInstance private (
lazy val defaultValueExpr = defaultValueExprArg
+ // This represents the default value at the start of processing, provided by
Review comment:
There are some instances where we are using the defaultValueExpression in places without access to a variable runtime data. For example, we check to see if a variable has a defaultValueExpression defined when resetting the instance to determine the correct state to reset to. We also evaluate the expressions in forceExpressionEvaluations without access to a VRD.
It may be possible to restructure these things, but I'm not sure its worth the effort.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602496405
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -117,6 +121,7 @@ class VariableInstance private (
def value = {
if (_value.isEmpty) {
_value = Some(valueArg)
+ firstInstanceInitialValue = valueArg
Review comment:
You are right, this isn't needed.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602496525
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -104,12 +102,17 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override def unparse(state: UState) = {
state.variableMap.newVariableInstance(context)
+
+ val v = state.variableMap.find(context.globalQName).getOrElse(Assert.invariantFailed("Unable to find variable %s".format(context.globalQName)))
Review comment:
Done
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r599183518
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section07/external_variables/external_variables.tdml
##########
@@ -338,9 +338,72 @@
<g xsi:type="xsd:int">48</g>
</ex:c>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:defineSchema name="v5">
+ <dfdl:defineVariable name="v_with_default" type="xsd:int"
+ defaultValue="1" external="true" />
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" />
Review comment:
Darn spaces vs. tabs
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r609609333
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -103,13 +101,17 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.variableMap.newVariableInstance(context)
+ val nvi = state.variableMap.newVariableInstance(context)
+
if (context.maybeDefaultValueExpr.isDefined) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
- val suspendableExpression = new NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get, context)
+ val dve = context.maybeDefaultValueExpr.get
+ nvi.setState(VariableInProcess)
+ val suspendableExpression = new NewVariableInstanceDefaultValueSuspendableExpression(dve, context, nvi)
suspendableExpression.run(state)
+ } else if (nvi.firstInstanceInitialValue.isDefined) {
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ nvi.setDefaultValue(nvi.firstInstanceInitialValue)
Review comment:
Codecov says this branch isn't covered. We should add a test for this. This is an important edge case we need to make sure is covered.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,123 +79,70 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
- def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- priorState: VariableState = VariableUndefined,
- priorValue: DataValuePrimitiveNullable = DataValue.NoValue) = {
-
- new VariableInstance(stateArg, valueArg, rd, defaultValueExprArg, priorState, priorValue)
+ def apply(rd: VariableRuntimeData) = {
+ new VariableInstance(rd)
}
}
/**
* See documentation for object VariableInstance
*/
-class VariableInstance private (
- @TransientParam stateArg: => VariableState,
- @TransientParam valueArg: => DataValuePrimitiveNullable,
- val rd: VariableRuntimeData,
- @TransientParam defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- var priorState: VariableState = VariableUndefined,
- var priorValue: DataValuePrimitiveNullable = DataValue.NoValue)
- extends Serializable
- with PreSerialization {
-
- lazy val defaultValueExpr = defaultValueExprArg
-
- private var _value: Option[DataValuePrimitiveNullable] = None
- private var _state: VariableState = null
-
- /**
- * Returns the current value of the VariableInstance
- *
- * This function allows value to be set while also enabling valueArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * value later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def value = {
- if (_value.isEmpty) {
- _value = Some(valueArg)
- }
-
- _value.get
- }
+class VariableInstance private (val rd: VariableRuntimeData)
+ extends Serializable {
- /**
- * Returns the current state of the VariableInstance
- *
- * This function allows state to be set while also enabling stateArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * state later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def state = {
- if (_state == null) {
- _state = stateArg
- }
+ var state: VariableState = VariableUndefined
+ var priorState: VariableState = VariableUndefined
+ var value: DataValuePrimitiveNullable = DataValue.NoValue
+ var priorValue: DataValuePrimitiveNullable = DataValue.NoValue
- _state
- }
+ // This represents the default value at the start of processing, provided by
+ // either the defaultValue expression or by an external binding
+ var firstInstanceInitialValue: DataValuePrimitiveNullable = DataValue.NoValue
def setState(s: VariableState) = {
this.priorState = this.state
- this._state = s
+ this.state = s
}
def setValue(v: DataValuePrimitiveNullable) = {
this.priorValue = this.value
- this._value = Some(v)
+ this.value = v
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
-
- override def preSerialization: Unit = {
- defaultValueExpr
- value
- state
+ /* This is used to set a default value without assigning a priorValue */
+ def setDefaultValue(v: DataValuePrimitiveNullable) = {
+ Assert.invariant((this.state == VariableUndefined || this.state == VariableInProcess) && v.isDefined)
+ this.state = VariableDefined
+ this.value = v
}
- override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s,%s)".format(
+ override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s)".format(
state,
value,
rd,
- defaultValueExpr,
+ rd.maybeDefaultValueExpr,
priorState,
- priorValue,
- this.hashCode)
+ priorValue)
def copy(
state: VariableState = state,
value: DataValuePrimitiveNullable = value,
rd: VariableRuntimeData = rd,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]] = defaultValueExpr,
priorState: VariableState = priorState,
priorValue: DataValuePrimitiveNullable = priorValue) = {
- val inst = new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
- inst.init()
+ val inst = new VariableInstance(rd)
+ inst.state = state
+ inst.priorState = priorState
+ inst.value = value
+ inst.priorValue = priorValue
inst
Review comment:
Now that we no onger need init(), I wonder if we should just make ``VariableInstance`` a case class now? Then we get this copy function for free and Scala ensures all state is copied (in fact, right now, we aren't copying firstInstanceInitialValue so there's potentially a bug). That also means we can get rid of the ``VariableInstance`` companion object since case class will also give us that for free.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r597822228
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -97,7 +98,8 @@ class VariableInstance private (
val rd: VariableRuntimeData,
@TransientParam defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
var priorState: VariableState = VariableUndefined,
- var priorValue: DataValuePrimitiveNullable = DataValue.NoValue)
+ var priorValue: DataValuePrimitiveNullable = DataValue.NoValue,
Review comment:
Ok. Thanks
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r601822815
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -105,6 +105,9 @@ class VariableInstance private (
lazy val defaultValueExpr = defaultValueExprArg
+ // This represents the default value at the start of processing, provided by
+ // either the defaultValue expression or by an external binding
+ var defaultValue: DataValuePrimitiveNullable = DataValue.NoValue
Review comment:
I think my most recent push addresses these changes, but it still feels a little awkward copying around the firstInstanceInitialValue as a NVI doesn't start with a copy of the original variable instance. Let me know what you think.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602412447
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -1,4 +1,5 @@
-/*
+/* This represents the default value at the start of processing, provided by
+ * either the defaultValue expression or by an external binding
* Licensed to the Apache Software Foundation (ASF) under one or more
Review comment:
As much as I love vim, it makes it easy for an inadvertent keystroke to do something like this. Missed it when I was reviewing the diff.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602309218
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -105,6 +105,9 @@ class VariableInstance private (
lazy val defaultValueExpr = defaultValueExprArg
+ // This represents the default value at the start of processing, provided by
+ // either the defaultValue expression or by an external binding
+ var defaultValue: DataValuePrimitiveNullable = DataValue.NoValue
Review comment:
Note, it would probably be worth added a comment to this affect, explaing why we have this initial thing and why we are copying. Assuming it stays and we don't come up with a better alternative.
--
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
[GitHub] [daffodil] jadams-tresys merged pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys merged pull request #510:
URL: https://github.com/apache/daffodil/pull/510
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r595223357
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/DFDLDefineVariable.scala
##########
@@ -137,6 +140,11 @@ final class DFDLNewVariableInstance(node: Node, decl: AnnotatedSchemaComponent)
case (Some(str), v) => schemaDefinitionError("Default value of variable was supplied both as attribute and element value: %s", node.toString)
}
+ final lazy val hasLocalDefault = (defaultValueAsAttribute, defaultValueAsElement) match {
+ case (None, "") => false
+ case _ => true
+ }
Review comment:
That's a good idea. I hadn't thought about treating it/returning a tuple, which is why I had it separate. Will make the change.
----------------------------------------------------------------
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608220936
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -103,13 +102,22 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.variableMap.newVariableInstance(context)
+ val v = state.variableMap.newVariableInstance(context)
+
if (context.maybeDefaultValueExpr.isDefined) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
- val suspendableExpression = new NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get, context)
- suspendableExpression.run(state)
+ val dve = context.maybeDefaultValueExpr.get
+ dve match {
+ case constExpr: ConstantExpression[_] => // Do nothing, value has already been computed at schema compile time
+ case _ => {
+ v.setState(VariableInProcess)
+ val suspendableExpression = new NewVariableInstanceDefaultValueSuspendableExpression(dve, context)
+ suspendableExpression.run(state)
+ }
+ }
+ } else {
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ v.setDefaultValue(v.firstInstanceInitialValue)
Review comment:
So we can actually just do a check to see if firstInstanceInitialValue is defined, which checks to see if it is something other than DataValue.NoValue. So no need for making it an Option.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602235703
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -104,12 +102,17 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override def unparse(state: UState) = {
state.variableMap.newVariableInstance(context)
+
+ val v = state.variableMap.find(context.globalQName).getOrElse(Assert.invariantFailed("Unable to find variable %s".format(context.globalQName)))
Review comment:
I notice this duplicate logic to find a varible that must exist here and above. Can newVariableInstance return the new instance? So we don't need to find it? And then can this instance also be passed as a parameter to NewVariableInstanceSuspendableExpression? I think this would avoid having to do this map lookup multiple times?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -1,4 +1,5 @@
-/*
+/* This represents the default value at the start of processing, provided by
+ * either the defaultValue expression or by an external binding
* Licensed to the Apache Software Foundation (ASF) under one or more
Review comment:
Looks like a comment was accidentaly inserted into the license.
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -84,10 +84,8 @@ final class NewVariableInstanceSuspendableExpression(
extends SuspendableExpression {
override protected def processExpressionResult(ustate: UState, v: DataValuePrimitive): Unit = {
- val vi = ustate.variableMap.find(rd.globalQName)
- Assert.invariant(vi.isDefined)
- vi.get.setState(VariableDefined)
- vi.get.setValue(v)
+ val vi = ustate.variableMap.find(rd.globalQName).getOrElse(Assert.invariantFailed("Unable to find variable %s".format(rd.globalQName)))
+ vi.setDefaultValue(v) // This also sets variable state to VariableDefined
Review comment:
I just realized this class is only to be used for setting a defaultValue in a newVariableInstance. Does it make sense to rename it so it's more clear, e.g. ``NewVariableInstanceDefaultValueSuspendableExpression``? That's pretty verbose, but does make the intention more clear. Or instead, just add a comment explaining that this suspension is only used for defaultValue in NVIs.
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -104,12 +102,17 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override def unparse(state: UState) = {
state.variableMap.newVariableInstance(context)
+
+ val v = state.variableMap.find(context.globalQName).getOrElse(Assert.invariantFailed("Unable to find variable %s".format(context.globalQName)))
+
if (context.maybeDefaultValueExpr.isDefined) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
+ v.setState(VariableInProcess)
val suspendableExpression = new NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get, context)
suspendableExpression.run(state)
+ } else {
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ v.setDefaultValue(v.firstInstanceInitialValue)
Review comment:
What if ``firstInstanceInitialValue`` was never set? I.e. a variable was not set externally and the defineVariable did not have a defaultValue? In that case, ``firstInstanceInitialValue`` is DataValue.NoValue? But that means right here we call ``setDefaultValue`` which will then set the value to ``Some(NoValue)`` and the state to ``VariableDefined``. So the state says it's defined but we don't actually have a value. Is that an expected state to be in? If not, maybe ``firstInstanceInitialValue`` need to be a Some/Maybe or something and we only call ``setDefaultValue`` if it's defined? That way this NVI stays in some undefined state with no value?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -328,10 +337,11 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
// Evaluate defineVariable statements with non-constant default value expressions
case (VariableDefined, DataValue.NoValue, true) => {
val res = inst.defaultValueExpr.get.evaluate(state)
- inst.setValue(DataValue.unsafeFromAnyRef(res))
+ inst.setDefaultValue(DataValue.unsafeFromAnyRef(res))
}
case (_, _, _) => // Do nothing
}
+ inst.firstInstanceInitialValue = inst.value
Review comment:
Personally, I'd rather see a function called after ``forceExprssionEvaluations`` is called that does this copy. That way there is only a single place where this variable is changed and it's easier to verify it's happening at the right time. Functionally it's exactly the same, but I think eaiser to read and understand what's going on.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -117,6 +121,7 @@ class VariableInstance private (
def value = {
if (_value.isEmpty) {
_value = Some(valueArg)
+ firstInstanceInitialValue = valueArg
Review comment:
Is this line needed? Shouldn't this variable only be set after all the external and initial defaultValue expressions are evaluated?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -148,22 +153,26 @@ class VariableInstance private (
this._value = Some(v)
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
+ /* This is used to set a default value without assigning a priorValue */
+ def setDefaultValue(v: DataValuePrimitiveNullable) = {
+ Assert.invariant(this._value.isEmpty || (this._value.get == DataValue.NoValue))
Review comment:
Odd to me that ``_value.get == NoValue`` is an allowed state. Almost feels like there should be an assertion that whenver we set ``_value`` to something that is isn't set to ``NoValue``. Also feels like there should be an assertion here that ``v != NoValue`` too? Unless Im not understanding how all these state changes work in this class. I'm getting a better understanding, but it's still not super obvious.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/ExpressionEvaluatingParsers.scala
##########
@@ -166,10 +166,16 @@ class NewVariableInstanceStartParser(override val context: VariableRuntimeData)
def parse(start: PState): Unit = {
start.newVariableInstance(context)
+
+ val v = start.variableMap.find(context.globalQName).getOrElse(Assert.invariantFailed("Unable to find variable %s".format(context.globalQName)))
+
if (context.maybeDefaultValueExpr.isDefined) {
- val v = start.variableMap.find(context.globalQName).get
val res = DataValue.unsafeFromAnyRef(context.maybeDefaultValueExpr.get.evaluate(start))
v.setDefaultValue(res)
+ } else {
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ v.setDefaultValue(v.firstInstanceInitialValue)
Review comment:
Same comments as unparse.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -103,6 +104,9 @@ class VariableInstance private (
lazy val defaultValueExpr = defaultValueExprArg
+ // This represents the default value at the start of processing, provided by
Review comment:
Just above this we have ``defaultValueExpr`` variable and ``defaultValueExprArg`` parameter. I'm wondering if with all this new logic those variables can be removed? Can we just get that expression off of the ``VariableRuntimeData`` when it's needed?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -328,10 +337,11 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
// Evaluate defineVariable statements with non-constant default value expressions
case (VariableDefined, DataValue.NoValue, true) => {
val res = inst.defaultValueExpr.get.evaluate(state)
- inst.setValue(DataValue.unsafeFromAnyRef(res))
+ inst.setDefaultValue(DataValue.unsafeFromAnyRef(res))
Review comment:
So this ``case`` has a VariableDefined state buy a value of NoValue? I think I'm missing something about how variables work because this feels like it should be an illegal state. How does a variable get in this state where it's defined with no value? I would expect this to be ``case(VariaableUndefined, _, true)``, so we only try to set the evaluate and set defaultValue if it has an expression and the state has't been set yet. This would also prevent the defultValue from overriding externally set vairables, but I guess in that case I guess the value wouldn't be NoValue so this case wouldn't get hit?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -462,27 +484,16 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
}
}
- /**
- * Creates a new instance of a variable with default value
- */
- def newVariableInstance(vrd: VariableRuntimeData, defaultValue: DataValuePrimitive) = {
- val varQName = vrd.globalQName
- val variableInstances = vTable.get(varQName)
- Assert.invariant(variableInstances.isDefined)
-
- variableInstances.get += vrd.createVariableInstance(VariableUtils.convert(defaultValue.getAnyRef.toString, vrd, vrd))
- }
-
/**
* Creates a new instance of a variable without default value
*/
def newVariableInstance(vrd: VariableRuntimeData) = {
val varQName = vrd.globalQName
val variableInstances = vTable.get(varQName)
Assert.invariant(variableInstances.isDefined)
-
- val v = vrd.createVariableInstance()
- variableInstances.get += v
+ val nvi = vrd.createVariableInstance()
+ nvi.firstInstanceInitialValue = getFirstInstance(varQName).get.firstInstanceInitialValue
Review comment:
Note that this doesn't necessarily need to come from the first instance. Since each instance gets a copy from the first, we can just copy the value from any previous instance. But could this just be
```scala
nvi.firstInstanceInitialValue = variableInstance(0).firstInstanceInitialValue
```
We don't really need ``getFirstInstance`` which is duplicating some effort, like the vtable lookup.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r610609616
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,123 +79,70 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
- def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- priorState: VariableState = VariableUndefined,
- priorValue: DataValuePrimitiveNullable = DataValue.NoValue) = {
-
- new VariableInstance(stateArg, valueArg, rd, defaultValueExprArg, priorState, priorValue)
+ def apply(rd: VariableRuntimeData) = {
+ new VariableInstance(rd)
}
}
/**
* See documentation for object VariableInstance
*/
-class VariableInstance private (
- @TransientParam stateArg: => VariableState,
- @TransientParam valueArg: => DataValuePrimitiveNullable,
- val rd: VariableRuntimeData,
- @TransientParam defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- var priorState: VariableState = VariableUndefined,
- var priorValue: DataValuePrimitiveNullable = DataValue.NoValue)
- extends Serializable
- with PreSerialization {
-
- lazy val defaultValueExpr = defaultValueExprArg
-
- private var _value: Option[DataValuePrimitiveNullable] = None
- private var _state: VariableState = null
-
- /**
- * Returns the current value of the VariableInstance
- *
- * This function allows value to be set while also enabling valueArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * value later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def value = {
- if (_value.isEmpty) {
- _value = Some(valueArg)
- }
-
- _value.get
- }
+class VariableInstance private (val rd: VariableRuntimeData)
+ extends Serializable {
- /**
- * Returns the current state of the VariableInstance
- *
- * This function allows state to be set while also enabling stateArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * state later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def state = {
- if (_state == null) {
- _state = stateArg
- }
+ var state: VariableState = VariableUndefined
+ var priorState: VariableState = VariableUndefined
+ var value: DataValuePrimitiveNullable = DataValue.NoValue
+ var priorValue: DataValuePrimitiveNullable = DataValue.NoValue
- _state
- }
+ // This represents the default value at the start of processing, provided by
+ // either the defaultValue expression or by an external binding
+ var firstInstanceInitialValue: DataValuePrimitiveNullable = DataValue.NoValue
def setState(s: VariableState) = {
this.priorState = this.state
- this._state = s
+ this.state = s
}
def setValue(v: DataValuePrimitiveNullable) = {
this.priorValue = this.value
- this._value = Some(v)
+ this.value = v
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
-
- override def preSerialization: Unit = {
- defaultValueExpr
- value
- state
+ /* This is used to set a default value without assigning a priorValue */
+ def setDefaultValue(v: DataValuePrimitiveNullable) = {
+ Assert.invariant((this.state == VariableUndefined || this.state == VariableInProcess) && v.isDefined)
+ this.state = VariableDefined
+ this.value = v
}
- override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s,%s)".format(
+ override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s)".format(
state,
value,
rd,
- defaultValueExpr,
+ rd.maybeDefaultValueExpr,
priorState,
- priorValue,
- this.hashCode)
+ priorValue)
def copy(
state: VariableState = state,
value: DataValuePrimitiveNullable = value,
rd: VariableRuntimeData = rd,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]] = defaultValueExpr,
priorState: VariableState = priorState,
priorValue: DataValuePrimitiveNullable = priorValue) = {
- val inst = new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
- inst.init()
+ val inst = new VariableInstance(rd)
+ inst.state = state
+ inst.priorState = priorState
+ inst.value = value
+ inst.priorValue = priorValue
inst
Review comment:
Done
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608254656
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -978,17 +978,12 @@ final class VariableRuntimeData(
@throws(classOf[java.io.IOException])
final private def writeObject(out: java.io.ObjectOutputStream): Unit = serializeObject(out)
- private lazy val state =
- if (!maybeDefaultValueExpr.isDefined) VariableUndefined
- else VariableDefined
-
- private lazy val value: DataValuePrimitiveNullable =
- if (maybeDefaultValueExpr.isEmpty) DataValue.NoValue
- else {
- val defaultValueExpr = maybeDefaultValueExpr.get
- defaultValueExpr match {
- case constExpr: ConstantExpression[_] => DataValue.unsafeFromAnyRef(constExpr.constant)
- case _ => DataValue.NoValue
+ private lazy val (state, value) = {
+ if (!maybeDefaultValueExpr.isDefined)
+ (VariableUndefined, DataValue.NoValue)
+ else maybeDefaultValueExpr.get match {
+ case constExpr: ConstantExpression[_] => (VariableDefined, DataValue.unsafeFromAnyRef(constExpr.constant))
+ case _ => (VariableUndefined, DataValue.NoValue)
Review comment:
Actually, looking at the tests that are failing, they are all external variable tests that don't use tdml runner. I'm wondering if these are simply relics of a time when TDMLRunner didn't support external variables. If so, could we just remove them (after assuring that there are equivalent TDML tests)?
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602410123
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -104,12 +102,17 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override def unparse(state: UState) = {
state.variableMap.newVariableInstance(context)
+
+ val v = state.variableMap.find(context.globalQName).getOrElse(Assert.invariantFailed("Unable to find variable %s".format(context.globalQName)))
+
if (context.maybeDefaultValueExpr.isDefined) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
+ v.setState(VariableInProcess)
val suspendableExpression = new NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get, context)
suspendableExpression.run(state)
+ } else {
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ v.setDefaultValue(v.firstInstanceInitialValue)
Review comment:
I'm pretty sure that a variable instance with value=NoValue and state=VariableDefined is normal and expected for a defineVariable without a default value. I believe only time a variable's state should be undefined is when the variable is first defined it's priorState is set to VariableUndefined. This comes into play when we are resetting a variable back to priorState/Value and we check to make sure that the variable is no undefined.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r610792309
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,123 +79,70 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
- def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- priorState: VariableState = VariableUndefined,
- priorValue: DataValuePrimitiveNullable = DataValue.NoValue) = {
-
- new VariableInstance(stateArg, valueArg, rd, defaultValueExprArg, priorState, priorValue)
+ def apply(rd: VariableRuntimeData) = {
+ new VariableInstance(rd)
}
}
/**
* See documentation for object VariableInstance
*/
-class VariableInstance private (
- @TransientParam stateArg: => VariableState,
- @TransientParam valueArg: => DataValuePrimitiveNullable,
- val rd: VariableRuntimeData,
- @TransientParam defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- var priorState: VariableState = VariableUndefined,
- var priorValue: DataValuePrimitiveNullable = DataValue.NoValue)
- extends Serializable
- with PreSerialization {
-
- lazy val defaultValueExpr = defaultValueExprArg
-
- private var _value: Option[DataValuePrimitiveNullable] = None
- private var _state: VariableState = null
-
- /**
- * Returns the current value of the VariableInstance
- *
- * This function allows value to be set while also enabling valueArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * value later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def value = {
- if (_value.isEmpty) {
- _value = Some(valueArg)
- }
-
- _value.get
- }
+class VariableInstance private (val rd: VariableRuntimeData)
+ extends Serializable {
- /**
- * Returns the current state of the VariableInstance
- *
- * This function allows state to be set while also enabling stateArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * state later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def state = {
- if (_state == null) {
- _state = stateArg
- }
+ var state: VariableState = VariableUndefined
+ var priorState: VariableState = VariableUndefined
+ var value: DataValuePrimitiveNullable = DataValue.NoValue
+ var priorValue: DataValuePrimitiveNullable = DataValue.NoValue
- _state
- }
+ // This represents the default value at the start of processing, provided by
+ // either the defaultValue expression or by an external binding
+ var firstInstanceInitialValue: DataValuePrimitiveNullable = DataValue.NoValue
def setState(s: VariableState) = {
this.priorState = this.state
- this._state = s
+ this.state = s
}
def setValue(v: DataValuePrimitiveNullable) = {
this.priorValue = this.value
- this._value = Some(v)
+ this.value = v
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
-
- override def preSerialization: Unit = {
- defaultValueExpr
- value
- state
+ /* This is used to set a default value without assigning a priorValue */
+ def setDefaultValue(v: DataValuePrimitiveNullable) = {
+ Assert.invariant((this.state == VariableUndefined || this.state == VariableInProcess) && v.isDefined)
+ this.state = VariableDefined
+ this.value = v
}
- override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s,%s)".format(
+ override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s)".format(
state,
value,
rd,
- defaultValueExpr,
+ rd.maybeDefaultValueExpr,
priorState,
- priorValue,
- this.hashCode)
+ priorValue)
def copy(
state: VariableState = state,
value: DataValuePrimitiveNullable = value,
rd: VariableRuntimeData = rd,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]] = defaultValueExpr,
priorState: VariableState = priorState,
priorValue: DataValuePrimitiveNullable = priorValue) = {
- val inst = new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
- inst.init()
+ val inst = new VariableInstance(rd)
+ inst.state = state
+ inst.priorState = priorState
+ inst.value = value
+ inst.priorValue = priorValue
inst
Review comment:
Just curious, what in VariableInstance needs a deep copy? Isn't all the state just enums and things that don't have a differencne between deep/shallow copy? Wonder if something else is going on?
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r595225692
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -104,7 +104,10 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override def unparse(state: UState) = {
state.variableMap.newVariableInstance(context)
- if (context.maybeDefaultValueExpr.isDefined) {
+
+ // Only process the default value expression if it is defined and only if
+ // the variable is non-external or has a local default
+ if (context.maybeDefaultValueExpr.isDefined && (!context.external || context.hasLocalDefault)) {
Review comment:
You are right, I missed that logic bug there. Will also add a test to check for this situation.
----------------------------------------------------------------
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r603454877
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -328,10 +337,11 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
// Evaluate defineVariable statements with non-constant default value expressions
case (VariableDefined, DataValue.NoValue, true) => {
val res = inst.defaultValueExpr.get.evaluate(state)
- inst.setValue(DataValue.unsafeFromAnyRef(res))
+ inst.setDefaultValue(DataValue.unsafeFromAnyRef(res))
Review comment:
Honestly, that is probably what the different states should be (only VariableDefined/Undefined are different than what you have). I don't think I've changed the current usage patterns of any of these states (other than adding VariableBeingDefined) while I've been making changes to variables, but it would probably be good to add clear documentation on what each state means and make the change to VariableDefined/Undefined. I don't think it will take much and shouldn't break anything.
I'll take a look at it tomorrow and try to add it to this commit, assuming it doesn't break things or require a large refactor.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r610777956
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -103,13 +101,17 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.variableMap.newVariableInstance(context)
+ val nvi = state.variableMap.newVariableInstance(context)
+
if (context.maybeDefaultValueExpr.isDefined) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
- val suspendableExpression = new NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get, context)
+ val dve = context.maybeDefaultValueExpr.get
+ nvi.setState(VariableInProcess)
+ val suspendableExpression = new NewVariableInstanceDefaultValueSuspendableExpression(dve, context, nvi)
suspendableExpression.run(state)
+ } else if (nvi.firstInstanceInitialValue.isDefined) {
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ nvi.setDefaultValue(nvi.firstInstanceInitialValue)
Review comment:
Digging into why this wasn't already covered lead me to finding a bug in unparsing. I wasn't forcing the evaluation of defaultValue expressions on defineVariable statements when unparsing. I've pushed up a change fixing that.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608251610
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -978,17 +978,12 @@ final class VariableRuntimeData(
@throws(classOf[java.io.IOException])
final private def writeObject(out: java.io.ObjectOutputStream): Unit = serializeObject(out)
- private lazy val state =
- if (!maybeDefaultValueExpr.isDefined) VariableUndefined
- else VariableDefined
-
- private lazy val value: DataValuePrimitiveNullable =
- if (maybeDefaultValueExpr.isEmpty) DataValue.NoValue
- else {
- val defaultValueExpr = maybeDefaultValueExpr.get
- defaultValueExpr match {
- case constExpr: ConstantExpression[_] => DataValue.unsafeFromAnyRef(constExpr.constant)
- case _ => DataValue.NoValue
+ private lazy val (state, value) = {
+ if (!maybeDefaultValueExpr.isDefined)
+ (VariableUndefined, DataValue.NoValue)
+ else maybeDefaultValueExpr.get match {
+ case constExpr: ConstantExpression[_] => (VariableDefined, DataValue.unsafeFromAnyRef(constExpr.constant))
+ case _ => (VariableUndefined, DataValue.NoValue)
Review comment:
One issue with this approach is that it does break some tests that relied on default values with constant expressions being evaluated at compile time (a few of the external variable loader unit tests (non-tdml). I'm going to push up a commit with them changed to not use default values.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602486054
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -328,10 +337,11 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
// Evaluate defineVariable statements with non-constant default value expressions
case (VariableDefined, DataValue.NoValue, true) => {
val res = inst.defaultValueExpr.get.evaluate(state)
- inst.setValue(DataValue.unsafeFromAnyRef(res))
+ inst.setDefaultValue(DataValue.unsafeFromAnyRef(res))
Review comment:
So VariableDefined with value=NoValue is end result of a defineVariable statement without a default value expression. In this instance, the variable is defined (hence the defineVariable statement) but without a value. I believe VariableUndefined is only used/checked when resetting a variable we check to make sure it is not undefined as that would mean we are trying to reset a variable that has already been reset.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608235666
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -978,17 +978,12 @@ final class VariableRuntimeData(
@throws(classOf[java.io.IOException])
final private def writeObject(out: java.io.ObjectOutputStream): Unit = serializeObject(out)
- private lazy val state =
- if (!maybeDefaultValueExpr.isDefined) VariableUndefined
- else VariableDefined
-
- private lazy val value: DataValuePrimitiveNullable =
- if (maybeDefaultValueExpr.isEmpty) DataValue.NoValue
- else {
- val defaultValueExpr = maybeDefaultValueExpr.get
- defaultValueExpr match {
- case constExpr: ConstantExpression[_] => DataValue.unsafeFromAnyRef(constExpr.constant)
- case _ => DataValue.NoValue
+ private lazy val (state, value) = {
+ if (!maybeDefaultValueExpr.isDefined)
+ (VariableUndefined, DataValue.NoValue)
+ else maybeDefaultValueExpr.get match {
+ case constExpr: ConstantExpression[_] => (VariableDefined, DataValue.unsafeFromAnyRef(constExpr.constant))
+ case _ => (VariableUndefined, DataValue.NoValue)
Review comment:
Also, just realized that removing the state/value stuff from RuntimeData.scala means we don't need to do the complicated/weird lazy argument stuff.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608226228
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -103,13 +102,22 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.variableMap.newVariableInstance(context)
+ val v = state.variableMap.newVariableInstance(context)
+
if (context.maybeDefaultValueExpr.isDefined) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
- val suspendableExpression = new NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get, context)
- suspendableExpression.run(state)
+ val dve = context.maybeDefaultValueExpr.get
+ dve match {
+ case constExpr: ConstantExpression[_] => // Do nothing, value has already been computed at schema compile time
+ case _ => {
+ v.setState(VariableInProcess)
+ val suspendableExpression = new NewVariableInstanceDefaultValueSuspendableExpression(dve, context)
+ suspendableExpression.run(state)
+ }
+ }
+ } else {
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ v.setDefaultValue(v.firstInstanceInitialValue)
Review comment:
Makes sense. I wonder if then it makes sense for _value to no longer be an Option then? Maybe ``def value`` and ``def stat`` just go away entirely if stateArg and valueArg aren't neeed anymore, and then ``value`` and ``state`` are just vars that are initlaized to NoValue and Undefined just like priorValue and priorState? Seems like _value only needed to be an option to deal with the pass by name of value, but I don't think we need that anymore?
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608984925
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,35 +82,26 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
+ state: VariableState = VariableUndefined,
+ value: DataValuePrimitiveNullable = DataValue.NoValue,
rd: VariableRuntimeData,
defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
Review comment:
Ah, I see what you mean now. I can just make the defaultValueExpr a class member and not worry about passing it in. I think that makes sense. In the VariableInstance contructor we will just set the defaultValueExpr class arg to what is in the VariableRuntimeData.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608221900
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -148,22 +168,26 @@ class VariableInstance private (
this._value = Some(v)
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
+ /* This is used to set a default value without assigning a priorValue */
+ def setDefaultValue(v: DataValuePrimitiveNullable) = {
+ Assert.invariant(this.state == VariableUndefined || this.state == VariableInProcess)
Review comment:
Yes, done.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r609616102
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,123 +79,70 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
- def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- priorState: VariableState = VariableUndefined,
- priorValue: DataValuePrimitiveNullable = DataValue.NoValue) = {
-
- new VariableInstance(stateArg, valueArg, rd, defaultValueExprArg, priorState, priorValue)
+ def apply(rd: VariableRuntimeData) = {
+ new VariableInstance(rd)
}
}
/**
* See documentation for object VariableInstance
*/
-class VariableInstance private (
- @TransientParam stateArg: => VariableState,
- @TransientParam valueArg: => DataValuePrimitiveNullable,
- val rd: VariableRuntimeData,
- @TransientParam defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- var priorState: VariableState = VariableUndefined,
- var priorValue: DataValuePrimitiveNullable = DataValue.NoValue)
- extends Serializable
- with PreSerialization {
-
- lazy val defaultValueExpr = defaultValueExprArg
-
- private var _value: Option[DataValuePrimitiveNullable] = None
- private var _state: VariableState = null
-
- /**
- * Returns the current value of the VariableInstance
- *
- * This function allows value to be set while also enabling valueArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * value later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def value = {
- if (_value.isEmpty) {
- _value = Some(valueArg)
- }
-
- _value.get
- }
+class VariableInstance private (val rd: VariableRuntimeData)
+ extends Serializable {
- /**
- * Returns the current state of the VariableInstance
- *
- * This function allows state to be set while also enabling stateArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * state later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def state = {
- if (_state == null) {
- _state = stateArg
- }
+ var state: VariableState = VariableUndefined
+ var priorState: VariableState = VariableUndefined
+ var value: DataValuePrimitiveNullable = DataValue.NoValue
+ var priorValue: DataValuePrimitiveNullable = DataValue.NoValue
- _state
- }
+ // This represents the default value at the start of processing, provided by
+ // either the defaultValue expression or by an external binding
+ var firstInstanceInitialValue: DataValuePrimitiveNullable = DataValue.NoValue
def setState(s: VariableState) = {
this.priorState = this.state
- this._state = s
+ this.state = s
}
def setValue(v: DataValuePrimitiveNullable) = {
this.priorValue = this.value
- this._value = Some(v)
+ this.value = v
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
-
- override def preSerialization: Unit = {
- defaultValueExpr
- value
- state
+ /* This is used to set a default value without assigning a priorValue */
+ def setDefaultValue(v: DataValuePrimitiveNullable) = {
+ Assert.invariant((this.state == VariableUndefined || this.state == VariableInProcess) && v.isDefined)
+ this.state = VariableDefined
+ this.value = v
}
- override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s,%s)".format(
+ override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s)".format(
state,
value,
rd,
- defaultValueExpr,
+ rd.maybeDefaultValueExpr,
priorState,
- priorValue,
- this.hashCode)
+ priorValue)
def copy(
state: VariableState = state,
value: DataValuePrimitiveNullable = value,
rd: VariableRuntimeData = rd,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]] = defaultValueExpr,
priorState: VariableState = priorState,
priorValue: DataValuePrimitiveNullable = priorValue) = {
- val inst = new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
- inst.init()
+ val inst = new VariableInstance(rd)
+ inst.state = state
+ inst.priorState = priorState
+ inst.value = value
+ inst.priorValue = priorValue
inst
Review comment:
Now that we no onger need init(), I wonder if we should just make ``VariableInstance`` a case class now? Then we get the copy function for free, so we can remove this copy function and Scala will ensure all state is copied (in fact, right now, we aren't copying firstInstanceInitialValue so there's potentially a bug). That also means we can get rid of the ``VariableInstance`` companion object since case class will also give us that for free.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r610778957
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,123 +79,70 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
- def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- priorState: VariableState = VariableUndefined,
- priorValue: DataValuePrimitiveNullable = DataValue.NoValue) = {
-
- new VariableInstance(stateArg, valueArg, rd, defaultValueExprArg, priorState, priorValue)
+ def apply(rd: VariableRuntimeData) = {
+ new VariableInstance(rd)
}
}
/**
* See documentation for object VariableInstance
*/
-class VariableInstance private (
- @TransientParam stateArg: => VariableState,
- @TransientParam valueArg: => DataValuePrimitiveNullable,
- val rd: VariableRuntimeData,
- @TransientParam defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- var priorState: VariableState = VariableUndefined,
- var priorValue: DataValuePrimitiveNullable = DataValue.NoValue)
- extends Serializable
- with PreSerialization {
-
- lazy val defaultValueExpr = defaultValueExprArg
-
- private var _value: Option[DataValuePrimitiveNullable] = None
- private var _state: VariableState = null
-
- /**
- * Returns the current value of the VariableInstance
- *
- * This function allows value to be set while also enabling valueArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * value later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def value = {
- if (_value.isEmpty) {
- _value = Some(valueArg)
- }
-
- _value.get
- }
+class VariableInstance private (val rd: VariableRuntimeData)
+ extends Serializable {
- /**
- * Returns the current state of the VariableInstance
- *
- * This function allows state to be set while also enabling stateArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * state later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def state = {
- if (_state == null) {
- _state = stateArg
- }
+ var state: VariableState = VariableUndefined
+ var priorState: VariableState = VariableUndefined
+ var value: DataValuePrimitiveNullable = DataValue.NoValue
+ var priorValue: DataValuePrimitiveNullable = DataValue.NoValue
- _state
- }
+ // This represents the default value at the start of processing, provided by
+ // either the defaultValue expression or by an external binding
+ var firstInstanceInitialValue: DataValuePrimitiveNullable = DataValue.NoValue
def setState(s: VariableState) = {
this.priorState = this.state
- this._state = s
+ this.state = s
}
def setValue(v: DataValuePrimitiveNullable) = {
this.priorValue = this.value
- this._value = Some(v)
+ this.value = v
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
-
- override def preSerialization: Unit = {
- defaultValueExpr
- value
- state
+ /* This is used to set a default value without assigning a priorValue */
+ def setDefaultValue(v: DataValuePrimitiveNullable) = {
+ Assert.invariant((this.state == VariableUndefined || this.state == VariableInProcess) && v.isDefined)
+ this.state = VariableDefined
+ this.value = v
}
- override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s,%s)".format(
+ override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s)".format(
state,
value,
rd,
- defaultValueExpr,
+ rd.maybeDefaultValueExpr,
priorState,
- priorValue,
- this.hashCode)
+ priorValue)
def copy(
state: VariableState = state,
value: DataValuePrimitiveNullable = value,
rd: VariableRuntimeData = rd,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]] = defaultValueExpr,
priorState: VariableState = priorState,
priorValue: DataValuePrimitiveNullable = priorValue) = {
- val inst = new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
- inst.init()
+ val inst = new VariableInstance(rd)
+ inst.state = state
+ inst.priorState = priorState
+ inst.value = value
+ inst.priorValue = priorValue
inst
Review comment:
Actually, I dug into this a bit more and it seems that when using a case class the copy method is only a shallow copy. This ends up causing several tests to break (weirdly none in the TestVariables TDML tests, which is why I thought this was an easy change). I've reverted back to using a normal class again.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r609090797
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,35 +82,26 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
+ state: VariableState = VariableUndefined,
+ value: DataValuePrimitiveNullable = DataValue.NoValue,
rd: VariableRuntimeData,
defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
Review comment:
You are right, we don't. I've made the change.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r596316123
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -97,7 +98,8 @@ class VariableInstance private (
val rd: VariableRuntimeData,
@TransientParam defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
var priorState: VariableState = VariableUndefined,
- var priorValue: DataValuePrimitiveNullable = DataValue.NoValue)
+ var priorValue: DataValuePrimitiveNullable = DataValue.NoValue,
Review comment:
priorValue... I'm struggling with this. Why do we need that? When we create a NVI, the default value does not use that of an enclosing NVI. The DFDL spec says "The tuple is initialized based on the specifics of the dfdl:defineVariable annotation." The "tuple" being the state of a variable instance. So why do we need priorValue?
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -104,7 +104,10 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override def unparse(state: UState) = {
state.variableMap.newVariableInstance(context)
- if (context.maybeDefaultValueExpr.isDefined) {
+
+ // Only process the default value expression if it is defined and only if
+ // the variable is non-external or has a local default
Review comment:
This comment is actually incorrect I think. For a newVariableInstance, we should always process the defaultValue expression if it is provided and the direction includes unparsing. If it is not provided, then the default value is that of the global defineVariable, which would have already been evaluated (at processor start time), or provided externally at start of processing for the global variable.
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section07/external_variables/external_variables.tdml
##########
@@ -338,9 +338,72 @@
<g xsi:type="xsd:int">48</g>
</ex:c>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:defineSchema name="v5">
+ <dfdl:defineVariable name="v_with_default" type="xsd:int"
+ defaultValue="1" external="true" />
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" />
Review comment:
Indentation
----------------------------------------------------------------
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608982392
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,35 +82,26 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
+ state: VariableState = VariableUndefined,
+ value: DataValuePrimitiveNullable = DataValue.NoValue,
rd: VariableRuntimeData,
defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
Review comment:
But VariantInstance has a reference to VRD, right? Can't those use that?
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602411057
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -84,10 +84,8 @@ final class NewVariableInstanceSuspendableExpression(
extends SuspendableExpression {
override protected def processExpressionResult(ustate: UState, v: DataValuePrimitive): Unit = {
- val vi = ustate.variableMap.find(rd.globalQName)
- Assert.invariant(vi.isDefined)
- vi.get.setState(VariableDefined)
- vi.get.setValue(v)
+ val vi = ustate.variableMap.find(rd.globalQName).getOrElse(Assert.invariantFailed("Unable to find variable %s".format(rd.globalQName)))
+ vi.setDefaultValue(v) // This also sets variable state to VariableDefined
Review comment:
I think a little more verbosity is fine. Change made.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602576685
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -328,10 +337,11 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
// Evaluate defineVariable statements with non-constant default value expressions
case (VariableDefined, DataValue.NoValue, true) => {
val res = inst.defaultValueExpr.get.evaluate(state)
- inst.setValue(DataValue.unsafeFromAnyRef(res))
+ inst.setDefaultValue(DataValue.unsafeFromAnyRef(res))
Review comment:
That feels like a bug to me, but maybe I just don't understand what the different states mean (we need to document them). My understanding of the states is:
<dl>
<dt>VariableUndefined</dt>
<dd>The variable has no value, a variable read is an error, setVariable is allowed</dd>
<dt>VariableBeingDefined</dt>
<dd>We are in the process of evaluting and setting the value to a default value, used for circularity detection of defineVariable defaulValue's</dd>
<dt>VariableDefined</dt>
<dd>The variable has a value (either default or externally set), a variable read of that value is allowed, set variable is allowed</dd>
<dt>VariableSet</dt>
<dd>the variable has a value that has been set via setVariable, variable read is allowed, setVariable is no longer allowed</dd>
<dt>VariableRead</dt>
<dd>the variable has been read with a value, that value is now set, variable read is allowed, setVariable is no longer allowed</dd>
<dt>VariableInProcess</dt>
<dd>A suspension to set a variable (via either defaultValue or setVariable?) during unparsing has been created. This variable does not yet have a value. All reads/sets of this variable should block until this state changes.
</dl>
This is my understanding, but it sounds like that's not the case?
Note that another comment I made suggested a change to so that VariableInstance's aren't initialize with NoValue, maybe that would affect some of this?
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608229709
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -978,17 +978,12 @@ final class VariableRuntimeData(
@throws(classOf[java.io.IOException])
final private def writeObject(out: java.io.ObjectOutputStream): Unit = serializeObject(out)
- private lazy val state =
- if (!maybeDefaultValueExpr.isDefined) VariableUndefined
- else VariableDefined
-
- private lazy val value: DataValuePrimitiveNullable =
- if (maybeDefaultValueExpr.isEmpty) DataValue.NoValue
- else {
- val defaultValueExpr = maybeDefaultValueExpr.get
- defaultValueExpr match {
- case constExpr: ConstantExpression[_] => DataValue.unsafeFromAnyRef(constExpr.constant)
- case _ => DataValue.NoValue
+ private lazy val (state, value) = {
+ if (!maybeDefaultValueExpr.isDefined)
+ (VariableUndefined, DataValue.NoValue)
+ else maybeDefaultValueExpr.get match {
+ case constExpr: ConstantExpression[_] => (VariableDefined, DataValue.unsafeFromAnyRef(constExpr.constant))
+ case _ => (VariableUndefined, DataValue.NoValue)
Review comment:
That makes sense to me. Most of the stuff in RuntimeData was set up to just get things working with the way we had been handling variables before when they were much simpler things without so much to keep track of.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r601784373
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -465,12 +488,12 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
/**
* Creates a new instance of a variable with default value
*/
- def newVariableInstance(vrd: VariableRuntimeData, defaultValue: DataValuePrimitive) = {
+ def newVariableInstance(vrd: VariableRuntimeData, defaultValue: DataValuePrimitiveNullable) = {
val varQName = vrd.globalQName
val variableInstances = vTable.get(varQName)
Assert.invariant(variableInstances.isDefined)
- variableInstances.get += vrd.createVariableInstance(VariableUtils.convert(defaultValue.getAnyRef.toString, vrd, vrd))
+ variableInstances.get += vrd.createVariableInstance(defaultValue)
}
Review comment:
Makes sense to me, will change.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608255192
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -978,17 +978,12 @@ final class VariableRuntimeData(
@throws(classOf[java.io.IOException])
final private def writeObject(out: java.io.ObjectOutputStream): Unit = serializeObject(out)
- private lazy val state =
- if (!maybeDefaultValueExpr.isDefined) VariableUndefined
- else VariableDefined
-
- private lazy val value: DataValuePrimitiveNullable =
- if (maybeDefaultValueExpr.isEmpty) DataValue.NoValue
- else {
- val defaultValueExpr = maybeDefaultValueExpr.get
- defaultValueExpr match {
- case constExpr: ConstantExpression[_] => DataValue.unsafeFromAnyRef(constExpr.constant)
- case _ => DataValue.NoValue
+ private lazy val (state, value) = {
+ if (!maybeDefaultValueExpr.isDefined)
+ (VariableUndefined, DataValue.NoValue)
+ else maybeDefaultValueExpr.get match {
+ case constExpr: ConstantExpression[_] => (VariableDefined, DataValue.unsafeFromAnyRef(constExpr.constant))
+ case _ => (VariableUndefined, DataValue.NoValue)
Review comment:
Doesn't seem unreasonable to me :+1:
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r599518803
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -103,17 +103,23 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.variableMap.newVariableInstance(context)
-
- // Only process the default value expression if it is defined and only if
- // the variable is non-external or has a local default
- if (context.maybeDefaultValueExpr.isDefined && (!context.external || context.hasLocalDefault)) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ val orig = state.variableMap.getFirstInstance(context.globalQName).get
+ state.variableMap.newVariableInstance(context, orig.defaultValue)
+
+ val v = state.variableMap.find(context.globalQName).getOrElse(Assert.invariantFailed("Unable to find variable %s".format(context.globalQName)))
+
+ // If the NVI has a default value expression, process it to replace the
+ // default value inherited from the global defineVariable statement.
+ if (context.maybeDefaultValueExpr.isDefined) {
+ v.setState(VariableInProcess)
val suspendableExpression = new NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get, context)
suspendableExpression.run(state)
}
+
+ if (v.value != DataValue.NoValue)
+ v.setState(VariableDefined)
Review comment:
Not sure I understand this, I think this needs a comment as to why we are doing this. If this instance has a value, why do we need to change it's state to defined. Shouldn't something else have done that when it was set?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -148,7 +154,11 @@ class VariableInstance private (
this._value = Some(v)
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
+ /* This is used to set a default value without assigning a priorValue */
+ def setDefaultValue(v: DataValuePrimitiveNullable) = {
+ this.defaultValue = v
+ this._value = Some(v)
Review comment:
Should this have an assertion that ``!this.value.isDefined``, that way we can ensure that we never change the default value after it already has a value, either via a setVariable or accidentally setting a defaultValue twice?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -105,6 +105,9 @@ class VariableInstance private (
lazy val defaultValueExpr = defaultValueExprArg
+ // This represents the default value at the start of processing, provided by
+ // either the defaultValue expression or by an external binding
+ var defaultValue: DataValuePrimitiveNullable = DataValue.NoValue
Review comment:
The comment on this new mamember only applies to the first instance, right? For all new variabe instances, this defaultValue is either a copy of the first instance, or is something complely different if it has its own defalutValue expression? And in the later case, we'll never actually use this value right? We only read this variable from the first instance? I think "defaulValue" is also maybe a bit confusing, since this could be an "external" value.
So, I'm wondering if it makes sense to rename this to something like ``firstInstanceInitialValue`` or something?Thisalso means that after we call ``createNewVariableInstance`` in ``newVariableInstance`` we could set this variable in the new instance to that of the previous instance. That way we are essentially just copying along the first intance value, and so if a NVI does need it, it can just get the value from its own ``firstInstanceInitialValue``.
I think this would also mean you don't need the ``defaultValue = valueArg`` or changes to setDefaultValue below? The less state changes, the better.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -547,7 +569,9 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
}
case _ => {
- variable.setValue(VariableUtils.convert(newValue.getAnyRef.toString, variable.rd, referringContext))
+ val value = VariableUtils.convert(newValue.getAnyRef.toString, variable.rd, referringContext)
+ variable.externalValue = value
Review comment:
I think this ``variable.externalValue = value`` goes away if the logic I mentioned previously makes sense to switch to. Again, one less place where we need to remember to change state.
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -103,17 +103,23 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.variableMap.newVariableInstance(context)
-
- // Only process the default value expression if it is defined and only if
- // the variable is non-external or has a local default
- if (context.maybeDefaultValueExpr.isDefined && (!context.external || context.hasLocalDefault)) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ val orig = state.variableMap.getFirstInstance(context.globalQName).get
+ state.variableMap.newVariableInstance(context, orig.defaultValue)
Review comment:
Isn't there a newVariableInstance function that doesn't require a default? So this could be something like:
```scala
state.variableMap.newVariableInstance(context)
if (context.maybeDefaultValueExpr.isDefined) {
// do suspension stuff
} else {
val orig = state.variableMap.getFirstInstance(context.globalQName).get
newInstance.setDefaultValue(orig.defaultValue)
}
```
This way we don't have to get the get the first instance or it's default if we have a defaultValue expression. And by not setting the default when we create the NVI, we aren't ever hitting a case where we set the default value only to change it later once the suspension evaluates.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -465,12 +488,12 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
/**
* Creates a new instance of a variable with default value
*/
- def newVariableInstance(vrd: VariableRuntimeData, defaultValue: DataValuePrimitive) = {
+ def newVariableInstance(vrd: VariableRuntimeData, defaultValue: DataValuePrimitiveNullable) = {
val varQName = vrd.globalQName
val variableInstances = vTable.get(varQName)
Assert.invariant(variableInstances.isDefined)
- variableInstances.get += vrd.createVariableInstance(VariableUtils.convert(defaultValue.getAnyRef.toString, vrd, vrd))
+ variableInstances.get += vrd.createVariableInstance(defaultValue)
}
Review comment:
I would actually prefer we drop this function all together. If I want to find all places where we set a defaultValue, I want to be able to grep for ``setDefaultValue``. Having to also grep for newVarialbeInstance where we pass in an extra paramter is more difficult. Is there any reason why uses of this can't be replaced with
```scala
val v = newVariableInstance(...)
v.setDefaultValue(...)
```
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/ExpressionEvaluatingParsers.scala
##########
@@ -165,15 +166,22 @@ class NewVariableInstanceStartParser(override val context: VariableRuntimeData)
override lazy val runtimeDependencies = Vector()
def parse(start: PState): Unit = {
- start.newVariableInstance(context)
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ val orig = start.variableMap.getFirstInstance(context.globalQName).get
+ start.newVariableInstance(context, orig.defaultValue)
- // Only process the default value expression if it is defined and only if
- // the variable is non-external or has a local default
- if (context.maybeDefaultValueExpr.isDefined && (!context.external || context.hasLocalDefault)) {
- val v = start.variableMap.find(context.globalQName).get
+ val v = start.variableMap.find(context.globalQName).getOrElse(Assert.invariantFailed("Unable to find variable %s".format(context.globalQName)))
+
+ // If the NVI has a default value expression, process it to replace the
+ // default value inherited from the global defineVariable statement.
+ if (context.maybeDefaultValueExpr.isDefined) {
val res = DataValue.unsafeFromAnyRef(context.maybeDefaultValueExpr.get.evaluate(start))
v.setDefaultValue(res)
}
+
+ if (v.value != DataValue.NoValue)
+ v.setState(VariableDefined)
Review comment:
Same comments for parse as unparse.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -82,7 +82,8 @@ object VariableInstance {
rd: VariableRuntimeData,
defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
priorState: VariableState = VariableUndefined,
- priorValue: DataValuePrimitiveNullable = DataValue.NoValue) = {
+ priorValue: DataValuePrimitiveNullable = DataValue.NoValue,
+ externalValue: DataValuePrimitiveNullable = DataValue.NoValue) = {
Review comment:
Based on other comment about switching to something like firstInstanceInitialValue, I'm wondering if externalValue shouldn't be passed in, since it's really only used for the first instance. This variable is just ignored in all other instances.
I'm wondering if the logic wants to be something like:
1. Early on, when parse() is called, call ``setDefaultValue() for all externalValues that are passed in.
2. Call forceExpressionEvaluations to evaluate all the defaultValues and call setDefaultValue(). At this point, all variables have a value from external, defaultValue, or do not have a value.
3. Call something that locks in all these initial values, something to the effect of ``variables.foreach { v.firstInstanceInitialValue = v.value }``
From this point on, the firstInstanceInitialValue variable never changes, except for copying it along to new instances. Less state changes is a good thing, which is my main concern with variable map stuff. There's so many state changes going on that I find it difficult to reason about its correctness. With this tweak to how things are set, I think state changes are a bit more clear where they take place. There's inidividual functions that specifcy where stage changes (e.g. forceExpressionEvaluations, lockInAllTheInitialValues(needs a better name)), and we avoid adding to many extra variables that are rarely used. There's so much state in this variable map stuff that the less variables and less places where changing of these variables takes place, the easier it becomes to reason about correctness.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -148,7 +154,11 @@ class VariableInstance private (
this._value = Some(v)
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
+ /* This is used to set a default value without assigning a priorValue */
Review comment:
I noticed forceExpressionEvaluations uses setValue. Should it be using setDefaultValue? Or is this something that should realy only be used at runtime?
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608222251
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -324,17 +348,31 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
// have default value expressions or are defined externally.
def forceExpressionEvaluations(state: ParseOrUnparseState): Unit = {
vTable.foreach { case (_, variableInstances) => { variableInstances.foreach { inst => {
- (inst.state, inst.value, inst.defaultValueExpr.isDefined) match {
+ (inst.state, inst.defaultValueExpr.isDefined) match {
// Evaluate defineVariable statements with non-constant default value expressions
- case (VariableDefined, DataValue.NoValue, true) => {
+ case (VariableUndefined, true) => {
val res = inst.defaultValueExpr.get.evaluate(state)
- inst.setValue(DataValue.unsafeFromAnyRef(res))
+ inst.setDefaultValue(DataValue.unsafeFromAnyRef(res))
}
- case (_, _, _) => // Do nothing
+ case (_, _) => // Do nothing
}
}}}}
}
+
+ /**
+ * This function is called immediately after forceExpressionEvaluations in
+ * order to set the firstInstanceInitialValue for each variable instance.
+ * These initial values will be inherited by future new variable instances if
+ * the newVariableInstance statement does not provide a default value
+ * expression
+ */
+ def setFirstInstanceInitialValues(): Unit = {
+ vTable.foreach { case (_, variableInstances) => {
+ variableInstances.foreach { inst => inst.firstInstanceInitialValue = inst.value }
Review comment:
Makes sense and I imagine that would be more performant anyway.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602489846
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -462,27 +484,16 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
}
}
- /**
- * Creates a new instance of a variable with default value
- */
- def newVariableInstance(vrd: VariableRuntimeData, defaultValue: DataValuePrimitive) = {
- val varQName = vrd.globalQName
- val variableInstances = vTable.get(varQName)
- Assert.invariant(variableInstances.isDefined)
-
- variableInstances.get += vrd.createVariableInstance(VariableUtils.convert(defaultValue.getAnyRef.toString, vrd, vrd))
- }
-
/**
* Creates a new instance of a variable without default value
*/
def newVariableInstance(vrd: VariableRuntimeData) = {
val varQName = vrd.globalQName
val variableInstances = vTable.get(varQName)
Assert.invariant(variableInstances.isDefined)
-
- val v = vrd.createVariableInstance()
- variableInstances.get += v
+ val nvi = vrd.createVariableInstance()
+ nvi.firstInstanceInitialValue = getFirstInstance(varQName).get.firstInstanceInitialValue
Review comment:
If we don't do the copy, then we get variables that have unset values--that feels dangerous to me. I'd rather have variables that we never use but have the right value, rather than variables that we might accientally use and have the wrong value.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r610800286
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,123 +79,70 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
- def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- priorState: VariableState = VariableUndefined,
- priorValue: DataValuePrimitiveNullable = DataValue.NoValue) = {
-
- new VariableInstance(stateArg, valueArg, rd, defaultValueExprArg, priorState, priorValue)
+ def apply(rd: VariableRuntimeData) = {
+ new VariableInstance(rd)
}
}
/**
* See documentation for object VariableInstance
*/
-class VariableInstance private (
- @TransientParam stateArg: => VariableState,
- @TransientParam valueArg: => DataValuePrimitiveNullable,
- val rd: VariableRuntimeData,
- @TransientParam defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- var priorState: VariableState = VariableUndefined,
- var priorValue: DataValuePrimitiveNullable = DataValue.NoValue)
- extends Serializable
- with PreSerialization {
-
- lazy val defaultValueExpr = defaultValueExprArg
-
- private var _value: Option[DataValuePrimitiveNullable] = None
- private var _state: VariableState = null
-
- /**
- * Returns the current value of the VariableInstance
- *
- * This function allows value to be set while also enabling valueArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * value later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def value = {
- if (_value.isEmpty) {
- _value = Some(valueArg)
- }
-
- _value.get
- }
+class VariableInstance private (val rd: VariableRuntimeData)
+ extends Serializable {
- /**
- * Returns the current state of the VariableInstance
- *
- * This function allows state to be set while also enabling stateArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * state later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def state = {
- if (_state == null) {
- _state = stateArg
- }
+ var state: VariableState = VariableUndefined
+ var priorState: VariableState = VariableUndefined
+ var value: DataValuePrimitiveNullable = DataValue.NoValue
+ var priorValue: DataValuePrimitiveNullable = DataValue.NoValue
- _state
- }
+ // This represents the default value at the start of processing, provided by
+ // either the defaultValue expression or by an external binding
+ var firstInstanceInitialValue: DataValuePrimitiveNullable = DataValue.NoValue
def setState(s: VariableState) = {
this.priorState = this.state
- this._state = s
+ this.state = s
}
def setValue(v: DataValuePrimitiveNullable) = {
this.priorValue = this.value
- this._value = Some(v)
+ this.value = v
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
-
- override def preSerialization: Unit = {
- defaultValueExpr
- value
- state
+ /* This is used to set a default value without assigning a priorValue */
+ def setDefaultValue(v: DataValuePrimitiveNullable) = {
+ Assert.invariant((this.state == VariableUndefined || this.state == VariableInProcess) && v.isDefined)
+ this.state = VariableDefined
+ this.value = v
}
- override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s,%s)".format(
+ override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s)".format(
state,
value,
rd,
- defaultValueExpr,
+ rd.maybeDefaultValueExpr,
priorState,
- priorValue,
- this.hashCode)
+ priorValue)
def copy(
state: VariableState = state,
value: DataValuePrimitiveNullable = value,
rd: VariableRuntimeData = rd,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]] = defaultValueExpr,
priorState: VariableState = priorState,
priorValue: DataValuePrimitiveNullable = priorValue) = {
- val inst = new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
- inst.init()
+ val inst = new VariableInstance(rd)
+ inst.state = state
+ inst.priorState = priorState
+ inst.value = value
+ inst.priorValue = priorValue
inst
Review comment:
Ah, nevermind, the default case class copy() doesn't copy member var's, only copies the constructor parameters. So all our var state isn't copied with a case class. You're right that we still need this copy method.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r610800764
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,123 +79,70 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
- def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- priorState: VariableState = VariableUndefined,
- priorValue: DataValuePrimitiveNullable = DataValue.NoValue) = {
-
- new VariableInstance(stateArg, valueArg, rd, defaultValueExprArg, priorState, priorValue)
+ def apply(rd: VariableRuntimeData) = {
+ new VariableInstance(rd)
}
}
/**
* See documentation for object VariableInstance
*/
-class VariableInstance private (
- @TransientParam stateArg: => VariableState,
- @TransientParam valueArg: => DataValuePrimitiveNullable,
- val rd: VariableRuntimeData,
- @TransientParam defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
- var priorState: VariableState = VariableUndefined,
- var priorValue: DataValuePrimitiveNullable = DataValue.NoValue)
- extends Serializable
- with PreSerialization {
-
- lazy val defaultValueExpr = defaultValueExprArg
-
- private var _value: Option[DataValuePrimitiveNullable] = None
- private var _state: VariableState = null
-
- /**
- * Returns the current value of the VariableInstance
- *
- * This function allows value to be set while also enabling valueArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * value later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def value = {
- if (_value.isEmpty) {
- _value = Some(valueArg)
- }
-
- _value.get
- }
+class VariableInstance private (val rd: VariableRuntimeData)
+ extends Serializable {
- /**
- * Returns the current state of the VariableInstance
- *
- * This function allows state to be set while also enabling stateArg to be
- * lazily evaluated. This allows the VariableInstance to support setting the
- * state later and allows the construction of the VariableMap containing the
- * VariableInstance before evaluating the default value expression
- */
- def state = {
- if (_state == null) {
- _state = stateArg
- }
+ var state: VariableState = VariableUndefined
+ var priorState: VariableState = VariableUndefined
+ var value: DataValuePrimitiveNullable = DataValue.NoValue
+ var priorValue: DataValuePrimitiveNullable = DataValue.NoValue
- _state
- }
+ // This represents the default value at the start of processing, provided by
+ // either the defaultValue expression or by an external binding
+ var firstInstanceInitialValue: DataValuePrimitiveNullable = DataValue.NoValue
def setState(s: VariableState) = {
this.priorState = this.state
- this._state = s
+ this.state = s
}
def setValue(v: DataValuePrimitiveNullable) = {
this.priorValue = this.value
- this._value = Some(v)
+ this.value = v
}
- def setDefaultValue(v: DataValuePrimitiveNullable) = this._value = Some(v)
-
- override def preSerialization: Unit = {
- defaultValueExpr
- value
- state
+ /* This is used to set a default value without assigning a priorValue */
+ def setDefaultValue(v: DataValuePrimitiveNullable) = {
+ Assert.invariant((this.state == VariableUndefined || this.state == VariableInProcess) && v.isDefined)
+ this.state = VariableDefined
+ this.value = v
}
- override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s,%s)".format(
+ override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s)".format(
state,
value,
rd,
- defaultValueExpr,
+ rd.maybeDefaultValueExpr,
priorState,
- priorValue,
- this.hashCode)
+ priorValue)
def copy(
state: VariableState = state,
value: DataValuePrimitiveNullable = value,
rd: VariableRuntimeData = rd,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]] = defaultValueExpr,
priorState: VariableState = priorState,
priorValue: DataValuePrimitiveNullable = priorValue) = {
- val inst = new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
- inst.init()
+ val inst = new VariableInstance(rd)
+ inst.state = state
+ inst.priorState = priorState
+ inst.value = value
+ inst.priorValue = priorValue
inst
Review comment:
Might be worth copying the firstInstanceInitialValue just for correctness though.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r608980547
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -65,35 +82,26 @@ case object VariableInProcess extends VariableState
/**
* Class for maintaining the state of a variable
- *
- * Note: stateArg, valueArg, and defaultValueExprArg are pass by name/lazy
- * values as it is necessary to postpone their evaluation beyond when the
- * VariableInstance is created. Attempting to evaluate these arguments will
- * likely trigger an expression to be evaluated, which will query the
- * VariableMap if the expression references another variable. If this is
- * occurring within a defineVariable call, the VariableMap hasn't been fully
- * created and attempting to evaluate such an expression will result in an OOLAG
- * Circular Definition Exception
*/
object VariableInstance {
def apply(
- stateArg: => VariableState,
- valueArg: => DataValuePrimitiveNullable,
+ state: VariableState = VariableUndefined,
+ value: DataValuePrimitiveNullable = DataValue.NoValue,
rd: VariableRuntimeData,
defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
Review comment:
There are a few places that it is still needed where we only have access to VariableInstances without a VariableRuntimeData. Things like VariableInstance.reset() and forceExpressionEvaluations()
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r604263416
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -328,10 +337,11 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
// Evaluate defineVariable statements with non-constant default value expressions
case (VariableDefined, DataValue.NoValue, true) => {
val res = inst.defaultValueExpr.get.evaluate(state)
- inst.setValue(DataValue.unsafeFromAnyRef(res))
+ inst.setDefaultValue(DataValue.unsafeFromAnyRef(res))
Review comment:
After taking another look at this, I think we may both have been confused here. This case of VariableDefined with DataValue.NoValue is used specifically when the defineVariable has a non-constant default value expression. The value is temporarily set to DataValue.NoValue until forceExpressionEvaluation actually evaluates the non-constant expression and sets the correct value. A more clear solution to this may be to have variables that have a non-constant default value expression remain Undefined until the expression has been evaluated. I'm testing how that change will look now.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r595154183
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -953,6 +953,7 @@ final class VariableRuntimeData(
pathArg: String,
namespacesArg: NamespaceBinding,
val external: Boolean,
+ val hasLocalDefault: Boolean,
val direction: VariableDirection,
@TransientParam maybeDefaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
Review comment:
I found this a little confusing at first, since it wasn't clear to me how maybeDefaultValueExprArg could be defined but not have a local default. But I think it's because if NVI doesn't have a defaultValue, then maybeDfaultValueExpr is set to the value of the defineVariable.maybeDefaultValueExpr. So that makes sense, but I'm wondering if it maybe a different variable name or comment might make that more clear? Maybe something like ``defaultValueIsLocal`` or something would make it clear that this default value expression might not be local?
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/DFDLDefineVariable.scala
##########
@@ -89,6 +91,7 @@ class DFDLDefineVariable(node: Node, doc: SchemaDocument)
this.path,
this.namespaces,
this.external,
+ this.hasLocalDefault,
Review comment:
I'm wondering if this should just always be true? Since the defaultValue of a defineVariable is always local? I guess that would maybe be confusing if it didn't have a defaultValue, but a rename (suggested below in another comment) of ``defaulValueIsLocal``, might make that more clear. That way if this has a defaultValue then defaultValueIsLocal determines if that value was local or not. But then if it doesn't have a defaultValue, then defalutValueIsLocal clearly doens't mean anything.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -482,6 +474,15 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
Assert.invariant(variableInstances.isDefined)
val v = vrd.createVariableInstance()
+
+ // If the variable is external, the original variable instance should have
+ // already been set to the externally provided value. Use this value as the
+ // default value
+ if (vrd.external && !vrd.hasLocalDefault) {
+ val value = variableInstances.get.last.externalValue
+ v.externalValue = value
+ v.setValue(value)
+ }
Review comment:
This sort of splits the NVI logic into two separate places, which I think makes it a bit harder to reason about. Part is here, and part is the actual parser/unparse? Thoughts on moving this logic into to NVI parser/unparse so it's all consolidated in one place?
Also, thoughts on changing the ``externalValue`` to be ``maybeExternalValue``, and anytime we create a new variable instance we just copy that value from the previous instance to the new instance (you may already do this). This way, we always have the external value if we need it, and you never need to look at previous onces. So the NVI logic just becomes something like
```scala
if (needToEvaluateDefaultExpression) {
// create suspension to set default value
} else if (v.maybeExternalValue.isDefined) {
v.setValue(v.maybeExerternalValue.get)
} else {
// no default anywhere and no external value, new instance remains undefined
}
```
This way, if logic about NVI's ever changes, we only have to change the one parser/unparser and don't also have to think about what other places might deal with external values/defaultExpressions/state/etc
Also note that this logic doesn't care if a variable is external or not. Only setExtVariable should care about that. Everything else can just assume if it has an external variable then it was allowed to be set.
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -104,7 +104,10 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override def unparse(state: UState) = {
state.variableMap.newVariableInstance(context)
- if (context.maybeDefaultValueExpr.isDefined) {
+
+ // Only process the default value expression if it is defined and only if
+ // the variable is non-external or has a local default
Review comment:
I don't think this comment adds much. All it really does it convert the if-expression to english, so it doesn't really add any clarification about why this check is here. I think it would be more help to explain why we have this logic. E.g.
> If the default expression is local, then we suspend and evaluate that. If it's not local, and the variable was set externally, then we use that external value. Otherwise, we use the default value expression from from the defineVariable, if it had one.
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/DFDLDefineVariable.scala
##########
@@ -137,6 +140,11 @@ final class DFDLNewVariableInstance(node: Node, decl: AnnotatedSchemaComponent)
case (Some(str), v) => schemaDefinitionError("Default value of variable was supplied both as attribute and element value: %s", node.toString)
}
+ final lazy val hasLocalDefault = (defaultValueAsAttribute, defaultValueAsElement) match {
+ case (None, "") => false
+ case _ => true
+ }
Review comment:
Thouts on combining this with defaultValue? I think that would help to self document that the defaultValue could come from the defaultValue of the defineVariable (which wasn't immediately clear to me a cause confusion). For example:
```scala
final lazy val (defaultValue, defaultValueIsLocal) = ... {
case (None, "") => (defv.defaultValue, false)
case ... => (Some(str), true)
...
}
```
##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/section07/external_variables/TestExternalVariables.scala
##########
@@ -56,6 +56,10 @@ class TestExternalVariables {
runner.runOneTest("override_define_vars_05")
}
+ @Test def test_override_define_vars_06(): Unit = {
+ runner.runOneTest("override_define_vars_06")
Review comment:
Can we also add this same test but with the config no set so we test the logic that if a variable is external, but no external variable is set then it uses the default value from define variable?
Might also be worth adding the test I mentioned in a comment to ensure we have the right logic for whether to evaluate a defineVariable defaultValue, or just use the value it evaluated to when parse/unaprse started.
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -104,7 +104,10 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override def unparse(state: UState) = {
state.variableMap.newVariableInstance(context)
- if (context.maybeDefaultValueExpr.isDefined) {
+
+ // Only process the default value expression if it is defined and only if
+ // the variable is non-external or has a local default
+ if (context.maybeDefaultValueExpr.isDefined && (!context.external || context.hasLocalDefault)) {
Review comment:
Note that this only checks if ``context.external`` is true. Should this check if it actually had a external value set? If a variable is external, but doesn't have an external value, then it should evaluate this expression.
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -104,7 +104,10 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override def unparse(state: UState) = {
state.variableMap.newVariableInstance(context)
- if (context.maybeDefaultValueExpr.isDefined) {
+
+ // Only process the default value expression if it is defined and only if
+ // the variable is non-external or has a local default
+ if (context.maybeDefaultValueExpr.isDefined && (!context.external || context.hasLocalDefault)) {
Review comment:
A question about the DFDL spec, it says regarding using the defalutValue from the defineVariable (so many subtleties).
> If the instance is not assigned a new default value then *it inherits the default value specified by dfdl:defineVariable* or externally provided by the DFDL processor.
Does this mean we should not be revaluating the expression of the inherited defineVariable, but should instead be using whatever the default value evaluated to? For example, say we have this:
```xml
<dfdl:defineVariable name="var1" type="xs:string" defaultValue="X" />
<dfdl:defineVariable name="var2" type="xs:string" defaultValue="{ $var1 }" />
...
<dfdl:newVariableInstance ref="var1" />
<dfd:setVariable ref="var1" value="Y" />
<dfdl:newVariableInstance ref="var2" />
```
So ``var1`` defaults to ``X``, as does ``var2`` since it defaults to the expression that gets the value of ``var1``.
Then we create an NVI of ``var1`` and set its value to ``Y``.
Then we create a NVI of ``var2`` with no local default. This NVI does not have a local defaultValue, and ``var2`` wasn't set externally, so we need to "inherit the default value specified by dfdl:defineVariable".
So what is actually inherited here? What should be the default value of this NVI ``var2``?
One interpretation is that we inherit the expression, and so we also need to reevaluate this expression to get the default. Since ``var1`` has a value of ``Y`` in this new scope, the ``var2`` defaultValue expression evautates to that value and it too should default to ``Y``.
Another interpretation is that we inherit the original value that the defaultValue originally evaluated to when we started the parse, and so it should default to ``X``.
I could see either interpretation as being valid, and I don't think the spec is 100% clear. I think maybe the former is more intutitive? Since the definition is saying "I want var2 to defalut to the value of var1". So if the value of var1 changes in a new scope, I would maybe expect that to continue to be the value of the in-scope var1. I think we currently implement the former.
----------------------------------------------------------------
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r610749034
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -103,6 +104,9 @@ class VariableInstance private (
lazy val defaultValueExpr = defaultValueExprArg
+ // This represents the default value at the start of processing, provided by
Review comment:
The VariableInstance constructor takes only a VRD now. So this thread seems to be resolved.
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -103,13 +101,17 @@ class NewVariableInstanceStartUnparser(override val context: VariableRuntimeData
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.variableMap.newVariableInstance(context)
+ val nvi = state.variableMap.newVariableInstance(context)
+
if (context.maybeDefaultValueExpr.isDefined) {
- val maybeVar = state.variableMap.find(context.globalQName)
- Assert.invariant(maybeVar.isDefined)
- maybeVar.get.setState(VariableInProcess)
- val suspendableExpression = new NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get, context)
+ val dve = context.maybeDefaultValueExpr.get
+ nvi.setState(VariableInProcess)
+ val suspendableExpression = new NewVariableInstanceDefaultValueSuspendableExpression(dve, context, nvi)
suspendableExpression.run(state)
+ } else if (nvi.firstInstanceInitialValue.isDefined) {
+ // The NVI will inherit the default value of the original variable instance
+ // This will also inherit any externally provided bindings.
+ nvi.setDefaultValue(nvi.firstInstanceInitialValue)
Review comment:
Still need this test case.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -328,10 +337,11 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
// Evaluate defineVariable statements with non-constant default value expressions
case (VariableDefined, DataValue.NoValue, true) => {
val res = inst.defaultValueExpr.get.evaluate(state)
- inst.setValue(DataValue.unsafeFromAnyRef(res))
+ inst.setDefaultValue(DataValue.unsafeFromAnyRef(res))
Review comment:
Please capture the comment above, clarifying the VariableDefined with DataValue.NoValue - what that means and how used, in code comments please. I suggest a comment is needed where VariableDefined is introduced.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r602488172
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -462,27 +484,16 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
}
}
- /**
- * Creates a new instance of a variable with default value
- */
- def newVariableInstance(vrd: VariableRuntimeData, defaultValue: DataValuePrimitive) = {
- val varQName = vrd.globalQName
- val variableInstances = vTable.get(varQName)
- Assert.invariant(variableInstances.isDefined)
-
- variableInstances.get += vrd.createVariableInstance(VariableUtils.convert(defaultValue.getAnyRef.toString, vrd, vrd))
- }
-
/**
* Creates a new instance of a variable without default value
*/
def newVariableInstance(vrd: VariableRuntimeData) = {
val varQName = vrd.globalQName
val variableInstances = vTable.get(varQName)
Assert.invariant(variableInstances.isDefined)
-
- val v = vrd.createVariableInstance()
- variableInstances.get += v
+ val nvi = vrd.createVariableInstance()
+ nvi.firstInstanceInitialValue = getFirstInstance(varQName).get.firstInstanceInitialValue
Review comment:
You are right that we don't need getFirstInstance anymore. I needed it before when I was setting the initial default from the NVI parser, but that has since been refactored.
That being said, I'm not sure you realized that newVariableInstance does not copy any data from the initial variable instance, so this copy is necessary.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #510: New variable instances should inherit external values
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #510:
URL: https://github.com/apache/daffodil/pull/510#discussion_r597806988
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -97,7 +98,8 @@ class VariableInstance private (
val rd: VariableRuntimeData,
@TransientParam defaultValueExprArg: => Maybe[CompiledExpression[AnyRef]],
var priorState: VariableState = VariableUndefined,
- var priorValue: DataValuePrimitiveNullable = DataValue.NoValue)
+ var priorValue: DataValuePrimitiveNullable = DataValue.NoValue,
Review comment:
Prior value is used in one place only right now. When we are backtracking from a point of uncertainty where a variable was changed, when the PState is reset we also reset all the changed variables back to their previous state/value.
--
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