mbeckerle commented on code in PR #1097:
URL: https://github.com/apache/daffodil/pull/1097#discussion_r1367418073
##########
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:
That PR has a lot of context for this.
Clearly not this change set's problem.
It is quite possible there are bugs in our variables implementation. We
don't have enough here to suggest creating a new JIRA ticket about any of this.
I'm marking this thread "resolved".
--
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]
