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 2021/11/24 21:12:48 UTC
[GitHub] [daffodil] mbeckerle opened a new pull request #686: WIP: Fix NVI so defaultValue expr has access to properties of term.
mbeckerle opened a new pull request #686:
URL: https://github.com/apache/daffodil/pull/686
@jadams-tresys You can pull this and add any other changes.
This is, I believe, part of the fix for DAFFODIL-2587.
Doesn't have a test showing that this fixes lookAhead when called from the default value of a newVariableInstance.
But it does pass the Term object so that the TermRuntimeData is available from the NewVariableInstanceStartParser/Unparser, and is used as the context when the expression is evaluated. So this *should* fix it.
Note that I did not make the corresponding change for SetVariable - which presumably also needs the Term so it can get access to the TermRuntimeData, so that bitOrder and other physical properties are available for functions like lookAhead.
DAFFODIL-2587
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #686: WIP: Fix NVI so defaultValue expr has access to properties of term.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #686:
URL: https://github.com/apache/daffodil/pull/686#discussion_r758469284
##########
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:
Yes, this will change where blame is placed for the error. Fixing that would require us to pass both the VRD and TRD down further into the software so that the blame can be on the VRD, even though things like bitOrder must come from the TRD.
Interesting that the def context is not covered by tests according to codeCov. I will see if I can contrive a test that will force it to be used.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #686: Fix NVI so defaultValue expr has access to properties of term.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #686:
URL: https://github.com/apache/daffodil/pull/686#discussion_r759468175
##########
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:
And another commit with tests specifically for getting an error from defaultvalue of dfdl:newVariableInstance.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #686: Fix NVI so defaultValue expr has access to properties of term.
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #686:
URL: https://github.com/apache/daffodil/pull/686#discussion_r759508105
##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/section23/dfdl_expressions/TestDFDLExpressions3.scala
##########
@@ -49,4 +49,13 @@ class TestDFDLExpressions3 {
// @Test def test_array_self_expr1() { runner.runOneTest("test_array_self_expr1") }
@Test def test_array_self_expr2(): Unit = { runner.runOneTest("test_array_self_expr2") }
+ @Test def test_setVariable_neg_01(): Unit = { runner.runOneTest("setVariable_neg_01") }
+
+ // DAFFODIL-2594
+ // @Test def test_setVariable_neg_line_info_01(): Unit = { runner.runOneTest("setVariable_neg_line_info_01") }
+
+ @Test def test_newVariableInstance_neg_01(): Unit = { runner.runOneTest("newVariableInstance_neg_01") }
+
+ // DAFFODIL-2594
+ @Test def test_newVariableInstance_neg_line_info_01(): Unit = { runner.runOneTest("newVariableInstance_neg_line_info_01") }
Review comment:
Was this supposed to be uncommented? It seems like the line info isn't being handled properly and causing the test to fail.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #686: WIP: Fix NVI so defaultValue expr has access to properties of term.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #686:
URL: https://github.com/apache/daffodil/pull/686#discussion_r758705252
##########
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:
Hmmm. The VRD actually just identifies what variable, right? I don't think there is a NVIRuntimeData object. Nor a SetVariableRuntimeData that contain the say, line number and file info for that schema component. So existing error messages are going to refer to the variable itself. Using the TermRuntimeData of the term is going to be a better error message than using the VRD (which is what we had been doing.)
There's a NonTermRuntimeData object which could potentially be the representative object for these NVI or SetVar statements. But I wouldn't change this currently.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #686: Fix NVI so defaultValue expr has access to properties of term.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #686:
URL: https://github.com/apache/daffodil/pull/686#discussion_r759470069
##########
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:
These errors *should* use the context member and so provide code coverage on those lines of the NVI implementation.
--
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
[GitHub] [daffodil] mbeckerle merged pull request #686: Fix NVI so defaultValue expr has access to properties of term.
Posted by GitBox <gi...@apache.org>.
mbeckerle merged pull request #686:
URL: https://github.com/apache/daffodil/pull/686
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #686: WIP: Fix NVI so defaultValue expr has access to properties of term.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #686:
URL: https://github.com/apache/daffodil/pull/686#discussion_r759457551
##########
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:
Another commit adds tests that divide by zero in a setVariable expression. We do get a diagnostic, but the behavior is poor as the line info is way off. Added DAFFODIL-2594 and a test that characterizes this for now.
This bug fix does allow dfdl:newVariableInstance and dfdl:setVariable to invoke dfdlx:lookAhead, so is worth having even if we fix the poor diagnostic behavior later.
--
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
[GitHub] [daffodil] jadams-tresys commented on a change in pull request #686: WIP: Fix NVI so defaultValue expr has access to properties of term.
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #686:
URL: https://github.com/apache/daffodil/pull/686#discussion_r758659308
##########
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:
I added some tests that I believe addresses the missing coverage on codeCov
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #686: WIP: Fix NVI so defaultValue expr has access to properties of term.
Posted by 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