jadams-tresys commented on code in PR #1097:
URL: https://github.com/apache/daffodil/pull/1097#discussion_r1367342148
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/VariableMap1.scala:
##########
@@ -431,59 +416,57 @@ class VariableMap private (vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance
pstate: ParseOrUnparseState,
) = {
val varQName = vrd.globalQName
- val variableInstances = vTable.get(varQName)
- if (variableInstances.isDefined) {
- val variable = variableInstances.get.last
- variable.state match {
- case VariableSet => {
- referringContext.SDE(
- "Cannot set variable %s twice. State was: %s. Existing value: %s",
- variable.rd.globalQName,
- VariableSet,
- variable.value,
- )
- }
+ val variableInstances = vTable(vrd.vmapIndex)
+ val variable = variableInstances.head
+ variable.state match {
+ case VariableSet => {
+ referringContext.SDE(
+ "Cannot set variable %s twice. State was: %s. Existing value: %s",
+ variable.rd.globalQName,
+ VariableSet,
+ variable.value,
+ )
+ }
- case VariableRead => {
+ case VariableRead => {
+
+ /**
+ * TODO: This should be an SDE, but due to a bug (DAFFODIL-1443) in
+ * the way we evaluate escapeSchemes it could lead us to setting the
+ * variable read too early */
+ pstate.SDW(
+ WarnID.VariableSet,
+ "Cannot set variable %s after reading the default value. State was:
%s. Existing value: %s",
+ variable.rd.globalQName,
+ VariableSet,
+ variable.value,
+ )
+ variable.setValue(newValue)
+ variable.setState(VariableSet)
+ }
+ case _ => {
+ vrd.direction match {
/**
- * TODO: This should be an SDE, but due to a bug (DAFFODIL-1443) in
- * the way we evaluate escapeSchemes it could lead us to setting the
- * variable read too early */
- pstate.SDW(
- WarnID.VariableSet,
- "Cannot set variable %s after reading the default value. State
was: %s. Existing value: %s",
- variable.rd.globalQName,
- VariableSet,
- variable.value,
- )
- variable.setValue(newValue)
- variable.setState(VariableSet)
- }
-
- case _ => {
- vrd.direction match {
- /**
- * Due to potential race conditions regarding the setting of
- * variables via setVariable and default values in combination with
- * suspensions during unparsing, we only allow the use of either
- * setVariable statements or a default value when unparsing a
- * variable.
- */
- case VariableDirection.UnparseOnly | VariableDirection.Both
- if (vrd.maybeDefaultValueExpr.isDefined &&
variableInstances.get.size > 1) => {
- // Variable has an unparse direction, a default value, and a
- // newVariableInstance
- pstate.SDE(
- "Variable %s has an unparse direction and a default value,
setting the variable may cause race conditions when combined with a forward
referencing expression.",
- varQName,
- )
- }
- case _ => // Do nothing
+ * Due to potential race conditions regarding the setting of
Review Comment:
I remember working on this for a while and it needs a lengthy test case in
order to hit this error message:
https://github.com/apache/daffodil/blob/716e5336621d6ec645eb4a3d3b47b12680a914ff/daffodil-test/src/test/resources/org/apache/daffodil/section07/variables/variables.tdml#L260
I cannot remember if we get indeterminate behavior from this test case if we
remove the error check here and I'm struggling to even understand how my test
is set up as it has several layers of nesting. Unfortunately I don't have time
to dig into this, but there might be some useful comments in the pull request
where this got merged:
https://github.com/apache/daffodil/pull/473
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
