[GitHub] [daffodil] stevedlawrence commented on a change in pull request #686: WIP: Fix NVI so defaultValue expr has access to properties of term.

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #686:
URL: https://github.com/apache/daffodil/pull/686#discussion_r758346440



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/ExpressionEvaluatingParsers.scala
##########
@@ -161,15 +162,18 @@ class SetVariableParser(expr: CompiledExpression[AnyRef], decl: VariableRuntimeD
   }
 }
 
-class NewVariableInstanceStartParser(override val context: VariableRuntimeData)
+final class NewVariableInstanceStartParser(vrd: VariableRuntimeData, trd: TermRuntimeData)
   extends PrimParser {
+
+  override def context = trd

Review comment:
       Any chance this could affect diagnostics? For example, if `evaluate` is called and it results in a runtime SDE, what gets used as the context for that SDE? If it uses this `context` variable then the SDE message might only include the term the NVI is placed on instead of the actual NVI element, which is maybe confusing? Reading the code, it's not exactly clear to me what a diagnostic would look like. Might be worth adding a test (or maybe we already have a test) to make sure the diagnostic is reasonable?




-- 
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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org