stevedlawrence commented on code in PR #987:
URL: https://github.com/apache/daffodil/pull/987#discussion_r1910875753
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/Parser.scala:
##########
@@ -227,26 +227,47 @@ abstract class CombinatorParser(override val context:
RuntimeData)
extends Parser
with CombinatorProcessor
-final class SeqCompParser(context: RuntimeData, val childParsers:
Vector[Parser])
- extends CombinatorParser(context) {
+final class SeqCompParser(
+ context: RuntimeData,
+ val childParsers: Vector[Parser],
+) extends CombinatorParser(context) {
override lazy val runtimeDependencies = Vector()
override def childProcessors = childParsers
override def nom = "seq"
- val numChildParsers = childParsers.size
+ val assertDiscrimExpressions = childParsers.collect { case ae:
AssertExpressionEvaluationParser => ae }
+ val discrimExpressions = assertDiscrimExpressions.filter { _.discrim }
+ val assertExpressions = assertDiscrimExpressions.filterNot { _.discrim }
+ val nonAssertChildren = childParsers.diff(assertExpressions)
Review Comment:
I think `nonAssertChildren` includes discriminator parsers so could
discriminators get evaluated twice, once in the first loop and once in the
discrimnator loop?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/Parser.scala:
##########
@@ -227,26 +227,47 @@ abstract class CombinatorParser(override val context:
RuntimeData)
extends Parser
with CombinatorProcessor
-final class SeqCompParser(context: RuntimeData, val childParsers:
Vector[Parser])
- extends CombinatorParser(context) {
+final class SeqCompParser(
+ context: RuntimeData,
+ val childParsers: Vector[Parser],
+) extends CombinatorParser(context) {
override lazy val runtimeDependencies = Vector()
override def childProcessors = childParsers
override def nom = "seq"
- val numChildParsers = childParsers.size
+ val assertDiscrimExpressions = childParsers.collect { case ae:
AssertExpressionEvaluationParser => ae }
+ val discrimExpressions = assertDiscrimExpressions.filter { _.discrim }
+ val assertExpressions = assertDiscrimExpressions.filterNot { _.discrim }
+ val nonAssertChildren = childParsers.diff(assertExpressions)
def parse(pstate: PState): Unit = {
- var i: Int = 0
- while (i < numChildParsers) {
- val parser = childParsers(i)
- parser.parse1(pstate)
- if (pstate.processorStatus ne Success)
- return
+ var i = 0
+
+ // Handle all non assert/discriminator child parsers first
+ while ((i < nonAssertChildren.size) && (pstate.processorStatus eq
Success)) {
+ nonAssertChildren(i).parse1(pstate)
+ i += 1
+ }
+
+ // Handle all discriminator expressions statements after sequence body
+ i = 0
+ while (i < discrimExpressions.size) {
+ val de = discrimExpressions(i)
+ if (pstate.processorStatus eq Success)
+ de.parse1(pstate)
+ else
+ pstate.withTempSuccess(de.parse1)
i += 1
}
- }
+ // Handle all non discriminator assert statements last
+ i = 0
+ while ((i < assertExpressions.size) && (pstate.processorStatus eq
Success)) {
+ assertExpressions(i).parse1(pstate)
+ i += 1
+ }
+ }
Review Comment:
Was this order confirmed by the DFDL-WG? First we do all normal parsers,
then the discriminator, and then assertions? And that the order has nothing to
do with the order things are defined in the schema?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/Parser.scala:
##########
@@ -227,26 +227,47 @@ abstract class CombinatorParser(override val context:
RuntimeData)
extends Parser
with CombinatorProcessor
-final class SeqCompParser(context: RuntimeData, val childParsers:
Vector[Parser])
- extends CombinatorParser(context) {
+final class SeqCompParser(
+ context: RuntimeData,
+ val childParsers: Vector[Parser],
+) extends CombinatorParser(context) {
override lazy val runtimeDependencies = Vector()
override def childProcessors = childParsers
override def nom = "seq"
- val numChildParsers = childParsers.size
+ val assertDiscrimExpressions = childParsers.collect { case ae:
AssertExpressionEvaluationParser => ae }
+ val discrimExpressions = assertDiscrimExpressions.filter { _.discrim }
+ val assertExpressions = assertDiscrimExpressions.filterNot { _.discrim }
+ val nonAssertChildren = childParsers.diff(assertExpressions)
def parse(pstate: PState): Unit = {
- var i: Int = 0
- while (i < numChildParsers) {
- val parser = childParsers(i)
- parser.parse1(pstate)
- if (pstate.processorStatus ne Success)
- return
+ var i = 0
+
+ // Handle all non assert/discriminator child parsers first
+ while ((i < nonAssertChildren.size) && (pstate.processorStatus eq
Success)) {
+ nonAssertChildren(i).parse1(pstate)
+ i += 1
+ }
+
+ // Handle all discriminator expressions statements after sequence body
+ i = 0
+ while (i < discrimExpressions.size) {
Review Comment:
There can only be one discriminator, right? This don't really need to be a
loop I think?
##########
daffodil-test/src/test/scala/org/apache/daffodil/section07/discriminators/TestDiscriminators.scala:
##########
@@ -79,7 +79,11 @@ class TestDiscriminators {
@Test def test_discrimExpression_03() = {
runner.runOneTest("discrimExpression_03") }
// DAFFODIL-1971
- // @Test def test_discrimExpression_04() = {
runner.runOneTest("discrimExpression_04") }
+ @Test def test_discrimExpression_04() = {
runner.runOneTest("discrimExpression_04") }
+ @Test def test_discrimExpression_05() = {
runner.runOneTest("discrimExpression_05") }
+ @Test def test_discrimExpression_06() = {
runner.runOneTest("discrimExpression_06") }
+ @Test def test_discrimExpression_07() = {
runner.runOneTest("discrimExpression_07") }
+ @Test def test_discrimExpression_08() = {
runner.runOneTest("discrimExpression_08") }
Review Comment:
You mentioned you ran things against the regression suite. Have you done so
with the recent changes? I'm hesitant to merge anything this close to release
that has significant changes to behavior that schemas might rely on. Though, I
won't block it if others are fine with the regressions.
--
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]
