stevedlawrence commented on a change in pull request #473:
URL: https://github.com/apache/incubator-daffodil/pull/473#discussion_r560992240
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -77,30 +76,52 @@ final class SetVariableUnparser(
}
+final class NewVariableInstanceSuspendableExpression(
+ override val expr: CompiledExpression[AnyRef],
+ override val rd: VariableRuntimeData)
+ extends SuspendableExpression {
+
+ override protected def processExpressionResult(ustate: UState, v:
DataValuePrimitive): Unit = {
+ ustate.variableMap.newVariableInstance(rd, v)
+ }
+
+ override protected def maybeKnownLengthInBits(ustate: UState) = MaybeULong(0)
+}
+
// When implemented this almost certainly wants to be a combinator
// Not two separate unparsers.
-class NewVariableInstanceStartUnparser(override val context: RuntimeData)
+class NewVariableInstanceStartUnparser(override val context:
VariableRuntimeData)
extends PrimUnparserNoData {
override lazy val runtimeDependencies = Vector()
override lazy val childProcessors = Vector()
+ def suspendableExpression = {
+ Assert.invariant(context.maybeDefaultValueExpr.isDefined)
+ new
NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get,
context)
+ }
+
override def unparse(state: UState) = {
- val vrd = context.asInstanceOf[VariableRuntimeData]
- state.newVariableInstance(vrd)
+ if (context.maybeDefaultValueExpr.isDefined) {
+ suspendableExpression.run(state)
+ }
+ else {
+ state.variableMap.newVariableInstance(context)
+ }
}
}
-class NewVariableInstanceEndUnparser(override val context: RuntimeData)
+class NewVariableInstanceEndUnparser(override val context: VariableRuntimeData)
extends PrimUnparserNoData {
override lazy val runtimeDependencies = Vector()
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.removeVariableInstance(context.asInstanceOf[VariableRuntimeData])
+ if (state.variableMap.find(context.globalQName).isDefined)
Review comment:
In what case coudl the variable not exist?
Is what's going on here is the newVariableInstance suspends and so hasn't
been created yet. We continue unparsing but because that instance wasn't
created so we have nothing to create so nothing to delete?
But, say we had two nested new variable instances. Say the outer one doesn't
suspend and the inner one does. In that case, the outer
NewVariableInstanceStartUnparser adds a new variable instance just fine. Then
we call NewVariableInstanceStartUnparser for the inner variable instance. That
inner NVI suspends and so doesn't create the variable isntance until much
later. Unparse will continue and we'll call NewVariableInstanceEndUnparser for
the inner one. Note that we still haven't created the outer new variable
instance. But we do have a new variable instance from the outter NVI. So this
find() will work, and we'll reomve a variable instance. But we'll remove the
outer NVI, which this should only remove the inner NVI (which doesn't exist
yet). So now variable access will likely fail because we've removed our
instance.
Does that all sound like what would happen, or am I missing something
subtle? If that is right, then it probably leads to my previous comment that we
do need to always create an NVI without an value, and then suspensions can
modify those values.
##########
File path:
daffodil-test/src/test/resources/org/apache/daffodil/section07/variables/variables.tdml
##########
@@ -2078,4 +2080,268 @@
</tdml:infoset>
</tdml:parserTestCase>
+
+ <tdml:defineSchema name="variable_direction_schema">
+ <xs:include
schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat"
+ representation="binary"/>
+
+ <dfdl:defineVariable name="remainingAddr" type="xs:string"
dfdlx:direction="unparseOnly" />
+ <xs:element name="root">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:annotation>
+ <xs:appinfo source="http://www.ogf.org/dfdl/";>
+ <dfdl:newVariableInstance ref="ex:remainingAddr" defaultValue="{
ex:IPsrc }" />
+ </xs:appinfo>
+ </xs:annotation>
+ <xs:element name="byte1" type="xs:unsignedByte"
dfdl:outputValueCalc="{ xs:unsignedByte(fn:substring-before($ex:remainingAddr,
'.')) }" />
+ <xs:sequence>
+ <xs:annotation>
+ <xs:appinfo source="http://www.ogf.org/dfdl/";>
+ <dfdl:newVariableInstance ref="ex:remainingAddr"
defaultValue="{ fn:substring-after($ex:remainingAddr, '.') }" />
+ </xs:appinfo>
+ </xs:annotation>
+ <xs:element name="byte2" type="xs:unsignedByte"
dfdl:outputValueCalc="{ xs:unsignedByte(fn:substring-before($ex:remainingAddr,
'.')) }" />
+ <xs:sequence>
+ <xs:annotation>
+ <xs:appinfo source="http://www.ogf.org/dfdl/";>
+ <dfdl:newVariableInstance ref="ex:remainingAddr"
defaultValue="{ fn:substring-after($ex:remainingAddr, '.') }" />
+ </xs:appinfo>
+ </xs:annotation>
+ <xs:element name="byte3" type="xs:unsignedByte"
dfdl:outputValueCalc="{ xs:unsignedByte(fn:substring-before($ex:remainingAddr,
'.')) }" />
+ <xs:sequence>
+ <xs:annotation>
+ <xs:appinfo source="http://www.ogf.org/dfdl/";>
+ <dfdl:newVariableInstance ref="ex:remainingAddr"
defaultValue="{ fn:substring-after($ex:remainingAddr, '.') }" />
+ </xs:appinfo>
+ </xs:annotation>
+ <xs:element name="byte4" type="xs:unsignedByte"
dfdl:outputValueCalc="{ xs:unsignedByte($ex:remainingAddr) }" />
+ </xs:sequence>
+ </xs:sequence>
+ </xs:sequence>
+ <xs:element name="IPsrc" type="xs:string" dfdl:lengthKind="explicit"
dfdl:length="7"
+ dfdl:inputValueCalc="{ fn:concat(../ex:byte1, '.', ../ex:byte2,
'.', ../ex:byte3, '.', ../ex:byte4) }" />
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
Review comment:
I think it might be worth adding anew test cases. SImilar to this where
we have nested NVI's, but where we access the variable after the each instance
goes out of scope. That way we can ensure that suspensions are happening
correctly and that we are properly removing NVI's from scopes correctly.
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -312,7 +314,7 @@ class VariableMap private(vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance]
def find(qName: GlobalQName): Option[VariableInstance] = {
val optBuf = vTable.get(qName)
val variab = {
- if (optBuf.isDefined)
+ if (optBuf.isDefined && !optBuf.get.isEmpty)
Review comment:
This feels like it shouldn't be needed and is maybe a sign that we're
doing somethign wrong?
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -344,6 +346,16 @@ class VariableMap private(vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance]
*/
def readVariable(vrd: VariableRuntimeData, referringContext: ThrowsSDE,
maybeState: Maybe[ParseOrUnparseState]): DataValuePrimitive = {
val varQName = vrd.globalQName
+ if (maybeState.isDefined) {
+ vrd.direction match {
+ case VariableDirection.ParseOnly if
(!maybeState.get.isInstanceOf[PState]) =>
+ maybeState.get.SDW("Attempting to read variable %s which is marked
as parseOnly during unparsing".format(varQName))
+ case VariableDirection.UnparseOnly if
(!maybeState.get.isInstanceOf[UState]) =>
+ maybeState.get.SDW("Attempting to read variable %s which is marked
as unparseOnly during parsing".format(varQName))
+ case _ => // Do nothing
Review comment:
Should these be SDEs?
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -77,30 +76,52 @@ final class SetVariableUnparser(
}
+final class NewVariableInstanceSuspendableExpression(
+ override val expr: CompiledExpression[AnyRef],
+ override val rd: VariableRuntimeData)
+ extends SuspendableExpression {
+
+ override protected def processExpressionResult(ustate: UState, v:
DataValuePrimitive): Unit = {
+ ustate.variableMap.newVariableInstance(rd, v)
+ }
+
+ override protected def maybeKnownLengthInBits(ustate: UState) = MaybeULong(0)
+}
+
Review comment:
Just curious, can you talk about the reason for this? Is this needed
because of the new direction stuff, or is it needed to fix a preexisting
newVariableInstance bug that this just happened to unconver?
Also, I'm unsure if this is right. This doesn't create a newVariableInstance
until *after* the expression result is evaluated. Which means this new instance
could not be created for a very long time while we wait for the expression
result to be evaluated. So what happens if a parser accesses that variable
before this suspendable expression evaluates and creates a new instance? Is it
going to access the old instance?
Almost seems like we need to create a newVariableInstance immediately but
without a value. And any accesses of that new instance need to suspend until
after this suspension evaluates and sets this value. I guess things could get
complicated because a setVariable could also try to change the value of this
newVariableInstance, and it too could suspend. So there needs to be some logic
about how which suspension gets precedence.
Or maybe I'm over compilicating this all?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
