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 2020/12/08 17:18:46 UTC
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #450: Improve newVariableInstace and defineVariable defaultValue expressions
mbeckerle commented on a change in pull request #450:
URL: https://github.com/apache/incubator-daffodil/pull/450#discussion_r538559080
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -58,48 +59,103 @@ case object VariableInProcess extends VariableState
/**
* Core tuple of a state for variables.
*/
-case class VariableInstance(
- var state: VariableState,
- var value: DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]],
+class VariableInstance private (
+ @TransientParam stateArg: => VariableState,
Review comment:
Again. Add a comment explaining that these by-name/lazy args are needed due to circularity between variable instances, and their default-value expressions, which want to be compiled, which requires a variable map (for constant folding), which requires the variable instances.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -58,48 +59,103 @@ case object VariableInProcess extends VariableState
/**
* Core tuple of a state for variables.
*/
-case class VariableInstance(
- var state: VariableState,
- var value: DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]],
+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 {
+ extends Serializable
+ with PreSerialization {
+
+ lazy val defaultValueExpr = defaultValueExprArg
+
+ private var _value: Option[DataValuePrimitiveNullable] = None
+ private var _state: VariableState = null
+
+ override def preSerialization: Unit = {
+ defaultValueExpr
+ value
+ state
+ }
+
+ override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s".format(
+ state,
+ value,
+ rd,
+ defaultValueExpr,
+ priorState,
+ priorValue)
+
+ def value = {
Review comment:
Add scaladoc explaining that this is a combination of a settable thing that is lazily evaluated to use the corresponding argument, but only if it has not been set. This allows the variable instance to support setting the value later, AND allow construction of the variable map containing the variable instance before evaluating the default value expression.
Also move these getter/setters so they and the private vars they modify are all adjacent in the code so that the pattern of private var, special getter, and setter becomes all in one spot.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -987,6 +988,12 @@ final class VariableRuntimeData(
}
}
- def createVariableInstance: VariableInstance = VariableInstance(state, value, this, maybeDefaultValueExpr)
+ def createVariableInstance(defaultValue: => DataValuePrimitiveNullable): VariableInstance = {
Review comment:
This usage of by-name/lazy arg needs a comment explaining why it is needed, which is to avoid a circularity where evaluating the expression needs the map that this variable instance needs to be part of.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -58,48 +59,103 @@ case object VariableInProcess extends VariableState
/**
* Core tuple of a state for variables.
*/
-case class VariableInstance(
- var state: VariableState,
- var value: DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]],
+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 {
+ extends Serializable
+ with PreSerialization {
+
+ lazy val defaultValueExpr = defaultValueExprArg
+
+ private var _value: Option[DataValuePrimitiveNullable] = None
+ private var _state: VariableState = null
+
+ override def preSerialization: Unit = {
+ defaultValueExpr
+ value
+ state
+ }
+
+ override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s".format(
+ state,
+ value,
+ rd,
+ defaultValueExpr,
+ priorState,
+ priorValue)
+
+ def value = {
+ if (_value.isEmpty) {
+ _value = Some(valueArg)
+ }
+
+ _value.get
+ }
+
+ def state = {
+ if (_state == null) {
+ _state = stateArg
+ }
+
+ _state
+ }
def copy(
state: VariableState = state,
value: DataValuePrimitiveNullable = value,
rd: VariableRuntimeData = rd,
defaultValueExpr: Maybe[CompiledExpression[AnyRef]] = defaultValueExpr,
priorState: VariableState = priorState,
- priorValue: DataValuePrimitiveNullable = priorValue) =
- new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
+ priorValue: DataValuePrimitiveNullable = priorValue) = {
+ val inst = new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
+ inst.init()
+ inst
+ }
+
+ def init() = preSerialization
def setState(s: VariableState) = {
this.priorState = this.state
- this.state = s
+ this._state = s
}
def setValue(v: DataValuePrimitiveNullable) = {
this.priorValue = this.value
- this.value = v
+ this._value = Some(v)
}
def reset() = {
Assert.invariant(this.state != VariableUndefined)
(this.state, this.priorState, this.defaultValueExpr.isDefined) match {
- case (VariableRead, VariableSet, _) => this.state = VariableSet
- case (VariableRead, _, true) => this.state = VariableDefined
+ case (VariableRead, VariableSet, _) => this._state = VariableSet
Review comment:
I suggest it is better to call the setState and setValue so that those are the only two methods that actually use the _state and _value members.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -58,48 +59,103 @@ case object VariableInProcess extends VariableState
/**
* Core tuple of a state for variables.
*/
-case class VariableInstance(
- var state: VariableState,
- var value: DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]],
+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 {
+ extends Serializable
+ with PreSerialization {
+
+ lazy val defaultValueExpr = defaultValueExprArg
+
+ private var _value: Option[DataValuePrimitiveNullable] = None
+ private var _state: VariableState = null
+
+ override def preSerialization: Unit = {
+ defaultValueExpr
+ value
+ state
+ }
+
+ override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s".format(
+ state,
+ value,
+ rd,
+ defaultValueExpr,
+ priorState,
+ priorValue)
+
+ def value = {
+ if (_value.isEmpty) {
+ _value = Some(valueArg)
+ }
+
+ _value.get
+ }
+
+ def state = {
+ if (_state == null) {
+ _state = stateArg
+ }
+
+ _state
+ }
def copy(
state: VariableState = state,
value: DataValuePrimitiveNullable = value,
rd: VariableRuntimeData = rd,
defaultValueExpr: Maybe[CompiledExpression[AnyRef]] = defaultValueExpr,
priorState: VariableState = priorState,
- priorValue: DataValuePrimitiveNullable = priorValue) =
- new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
+ priorValue: DataValuePrimitiveNullable = priorValue) = {
+ val inst = new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
+ inst.init()
+ inst
+ }
+
+ def init() = preSerialization
def setState(s: VariableState) = {
this.priorState = this.state
- this.state = s
+ this._state = s
}
def setValue(v: DataValuePrimitiveNullable) = {
this.priorValue = this.value
- this.value = v
+ this._value = Some(v)
}
def reset() = {
Assert.invariant(this.state != VariableUndefined)
(this.state, this.priorState, this.defaultValueExpr.isDefined) match {
- case (VariableRead, VariableSet, _) => this.state = VariableSet
- case (VariableRead, _, true) => this.state = VariableDefined
+ case (VariableRead, VariableSet, _) => this._state = VariableSet
+ case (VariableRead, _, true) => this._state = VariableDefined
case (VariableSet, _, _) => {
- this.state = this.priorState
- this.value = this.priorValue
+ this._state = this.priorState
+ this._value = Some(this.priorValue)
}
case (_, _, _) => Assert.impossible("Should have SDE before reaching this")
}
}
}
+object VariableInstance {
+ def apply(
+ stateArg: => VariableState,
Review comment:
Move so this is adjacent to the actual class constructor (move to directly above). Then the scaladoc for this should have doc about why lazy/byname passing of args, since this is actually the public method that can be called by users of this class. The private class constructor can have scaladoc that refers to this method's doc rather than repeating it.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -128,6 +184,10 @@ class VariableHasNoValue(qname: NamedQName, context: VariableRuntimeData) extend
"Variable map (runtime): variable %s has no value. It was not set, and has no default value.".format(qname))
with RetryableException
+class VariableCircularDefinition(qname: NamedQName, context: VariableRuntimeData) extends VariableException(qname, context,
+ "Variable map (runtime): variable %s is part of a circular definition with other variables".format(qname))
+ with RetryableException
Review comment:
Hmmm. Why is this retryable? At unparse time, can variables refer to other variables and end up in a cycle? I guess a newVariableInstance with a default value expression that references another variable, which is also defined by a newVariableInstance... those wouldn't be statically evaluated because they're not default values of the defineVariable declaration/definition, so they would be evaluated entirely at unparse time, and would be a cycle.
I think it is worth adding scaladoc that explains when this exception is thrown - at start time if driven from defineVariable expressions being circular - or later during runtime if via newVariableInstance is the source.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -58,48 +59,103 @@ case object VariableInProcess extends VariableState
/**
* Core tuple of a state for variables.
*/
-case class VariableInstance(
- var state: VariableState,
- var value: DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]],
+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 {
+ extends Serializable
+ with PreSerialization {
+
+ lazy val defaultValueExpr = defaultValueExprArg
+
+ private var _value: Option[DataValuePrimitiveNullable] = None
+ private var _state: VariableState = null
+
+ override def preSerialization: Unit = {
+ defaultValueExpr
+ value
+ state
+ }
+
+ override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s".format(
+ state,
+ value,
+ rd,
+ defaultValueExpr,
+ priorState,
+ priorValue)
+
+ def value = {
+ if (_value.isEmpty) {
+ _value = Some(valueArg)
+ }
+
+ _value.get
+ }
+
+ def state = {
+ if (_state == null) {
+ _state = stateArg
+ }
+
+ _state
+ }
def copy(
state: VariableState = state,
value: DataValuePrimitiveNullable = value,
rd: VariableRuntimeData = rd,
defaultValueExpr: Maybe[CompiledExpression[AnyRef]] = defaultValueExpr,
priorState: VariableState = priorState,
- priorValue: DataValuePrimitiveNullable = priorValue) =
- new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
+ priorValue: DataValuePrimitiveNullable = priorValue) = {
+ val inst = new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
+ inst.init()
+ inst
+ }
+
+ def init() = preSerialization
def setState(s: VariableState) = {
this.priorState = this.state
- this.state = s
+ this._state = s
}
def setValue(v: DataValuePrimitiveNullable) = {
Review comment:
This one too.
##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/section07/variables/TestVariables.scala
##########
@@ -59,6 +59,16 @@ class TestVariables {
@Test def test_varInstance_09(): Unit = { runner.runOneTest("varInstance_09") }
@Test def test_varInstance_10(): Unit = { runner.runOneTest("varInstance_10") }
@Test def test_varInstance_11(): Unit = { runner.runOneTest("varInstance_11") }
+ @Test def test_varInstance_12(): Unit = { runner.runOneTest("varInstance_12") }
+
+ @Test def test_defineVariable_nonConstantExpression(): Unit = { runner.runOneTest("defineVariable_nonConstantExpression") }
+ // The following tests don't seem to play nicely with TDMLRunner and the errors are not detected/checked against
+ //@Test def test_circular_defineVariable_err(): Unit = { runner.runOneTest("circular_defineVariable_err") }
Review comment:
Is there still a reason for these tests to be commented out? If so please create JIRA tickets for these.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -58,48 +59,103 @@ case object VariableInProcess extends VariableState
/**
* Core tuple of a state for variables.
*/
-case class VariableInstance(
- var state: VariableState,
- var value: DataValuePrimitiveNullable,
- rd: VariableRuntimeData,
- defaultValueExpr: Maybe[CompiledExpression[AnyRef]],
+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 {
+ extends Serializable
+ with PreSerialization {
+
+ lazy val defaultValueExpr = defaultValueExprArg
+
+ private var _value: Option[DataValuePrimitiveNullable] = None
+ private var _state: VariableState = null
+
+ override def preSerialization: Unit = {
+ defaultValueExpr
+ value
+ state
+ }
+
+ override def toString: String = "VariableInstance(%s,%s,%s,%s,%s,%s".format(
+ state,
+ value,
+ rd,
+ defaultValueExpr,
+ priorState,
+ priorValue)
+
+ def value = {
+ if (_value.isEmpty) {
+ _value = Some(valueArg)
+ }
+
+ _value.get
+ }
+
+ def state = {
+ if (_state == null) {
+ _state = stateArg
+ }
+
+ _state
+ }
def copy(
state: VariableState = state,
value: DataValuePrimitiveNullable = value,
rd: VariableRuntimeData = rd,
defaultValueExpr: Maybe[CompiledExpression[AnyRef]] = defaultValueExpr,
priorState: VariableState = priorState,
- priorValue: DataValuePrimitiveNullable = priorValue) =
- new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
+ priorValue: DataValuePrimitiveNullable = priorValue) = {
+ val inst = new VariableInstance(state, value, rd, defaultValueExpr, priorState, priorValue)
+ inst.init()
+ inst
+ }
+
+ def init() = preSerialization
def setState(s: VariableState) = {
Review comment:
Move this setter up above the copy method so the getter and setter are adjacent in the code.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -222,7 +300,7 @@ class VariableMap private(vTable: Map[GlobalQName, MStackOf[VariableInstance]])
/**
* For testing mostly.
Review comment:
Mostly? Is it used outside of testing or not? Please clarify.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -167,16 +227,16 @@ final class VariableBox(initialVMap: VariableMap) {
* The DPath implementation must be made to implement the
* no-set-after-default-value-has-been-read behavior. This requires that reading the variables causes a state transition.
*/
-class VariableMap private(vTable: Map[GlobalQName, MStackOf[VariableInstance]])
+class VariableMap private(vTable: Map[GlobalQName, Queue[VariableInstance]])
extends Serializable {
def this(topLevelVRDs: Seq[VariableRuntimeData] = Nil) =
this(Map(topLevelVRDs.map {
vrd =>
- val variab = vrd.createVariableInstance
- val stack = new MStackOf[VariableInstance]
- stack.push(variab)
- (vrd.globalQName, stack)
+ val variab = vrd.createVariableInstance()
+ val queue = new Queue[VariableInstance]
Review comment:
Add comment about why using a Queue here, vs. MStack, which I believe is serializability
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -222,7 +300,7 @@ class VariableMap private(vTable: Map[GlobalQName, MStackOf[VariableInstance]])
/**
* For testing mostly.
*/
- def getVariableBindings(qn: GlobalQName): MStackOf[VariableInstance] = {
+ def getVariableBindings(qn: GlobalQName): Queue[VariableInstance] = {
Review comment:
We used to use a stack data structure. Why is this Queue and not Stack? It would seem the most natural replacement for MStack would be Stack.
This would eliminate a bunch of confusion below where we get a "queue" from the vtable using a "qname", i.e., where "Q" gets overloaded about Qualified (as in names) and Queue (as in data structure).
Can we use Stack?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -232,19 +310,37 @@ class VariableMap private(vTable: Map[GlobalQName, MStackOf[VariableInstance]])
*/
def readVariable(vrd: VariableRuntimeData, referringContext: ThrowsSDE, maybePstate: Maybe[ParseOrUnparseState]): DataValuePrimitive = {
val varQName = vrd.globalQName
- val stack = vTable.get(varQName)
- if (stack.isDefined) {
- val variable = stack.get.top
- variable.state match {
- case VariableRead if (variable.value.isDefined) => variable.value.getNonNullable
- case VariableDefined | VariableSet if (variable.value.isDefined) => {
+ val queue = vTable.get(varQName)
+ if (queue.isDefined) {
+ val variable = queue.get.head
+ (variable.state, variable.value.isDefined, variable.defaultValueExpr.isDefined) match {
+ case (VariableRead, true, _) => variable.value.getNonNullable
+ case (VariableDefined | VariableSet, true, _) => {
if (maybePstate.isDefined && maybePstate.get.isInstanceOf[PState])
maybePstate.get.asInstanceOf[PState].markVariableRead(vrd)
variable.setState(VariableRead)
variable.value.getNonNullable
}
- case _ => throw new VariableHasNoValue(varQName, vrd)
+ case (VariableDefined, false, true) => {
+ variable.setState(VariableInProcess)
+ if (maybePstate.isDefined && maybePstate.get.isInstanceOf[PState]) {
+ val pstate = maybePstate.get.asInstanceOf[PState]
+ val res = DataValue.unsafeFromAnyRef(variable.defaultValueExpr.get.evaluate(pstate))
+
+ // Need to update the variable's value with the result of the
+ // expression
+ variable.setState(VariableRead)
+ variable.setValue(res)
+
+ res
+ } else {
+ variable.setState(VariableRead)
+ variable.value.getNonNullable
Review comment:
The codecov warnings here are useful. I haven't seen these show up inline in a code review before.
These branches in the code do actually need tests to convince us the logic here is correct.
----------------------------------------------------------------
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