stevedlawrence commented on code in PR #987:
URL: https://github.com/apache/daffodil/pull/987#discussion_r2308307089
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/Parser.scala:
##########
@@ -227,26 +227,25 @@ abstract class CombinatorParser(override val context:
RuntimeData)
extends Parser
with CombinatorProcessor
-final class SeqCompParser(context: RuntimeData, val childParsers:
Array[Parser])
- extends CombinatorParser(context) {
+final class SeqCompParser(
+ context: RuntimeData,
+ val childParsers: Array[Parser],
+ testAssert: Array[Parser]
+) extends CombinatorParser(context) {
override def runtimeDependencies = Vector()
override def childProcessors = childParsers.toVector
override def nom = "seq"
- val numChildParsers = childParsers.size
-
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
- i += 1
+ childParsers.foreach { cp =>
Review Comment:
I think we intentionally avoided childParsers.foreach since I think we found
that allocated lambda expression and showed up in profiling. I'm not sure if
scala3 changed that at all, but we should probably stick what the for loop
without profiling to figure it out
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/Parser.scala:
##########
@@ -227,26 +227,25 @@ abstract class CombinatorParser(override val context:
RuntimeData)
extends Parser
with CombinatorProcessor
-final class SeqCompParser(context: RuntimeData, val childParsers:
Array[Parser])
- extends CombinatorParser(context) {
+final class SeqCompParser(
+ context: RuntimeData,
+ val childParsers: Array[Parser],
+ testAssert: Array[Parser]
Review Comment:
This isn't used anymore, I think it can be removed.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/DFDLStatementMixin.scala:
##########
@@ -213,10 +213,11 @@ trait ProvidesDFDLStatementMixin extends ThrowsSDE with
HasTermCheck {
st.testKind != TestKind.Pattern
}
- final lazy val patternStatements: Seq[DFDLStatement] = patternAsserts ++
patternDiscrims
+ // Discriminator statements must be before asserts
+ final lazy val patternStatements: Seq[DFDLStatement] = patternDiscrims ++
patternAsserts
- final lazy val lowPriorityStatements: Seq[DFDLStatement] =
- setVariableStatements ++ nonPatternAsserts ++ nonPatternDiscrims
+ final lazy val assertDiscrimExpressionStatements: Seq[DFDLStatement] =
+ nonPatternDiscrims ++ nonPatternAsserts
Review Comment:
I think this loses the order that asserts and discriminators are defined,
always evaluating discriminators before asserts. I think if we are to obey the
order they are defined in the schema we really can't separate discriminators
and asserts, we kindof have to treat them as one group of things.
That said, the checkDiscriminatorsAssertsDisjoint function below has this
check:
```scala
schemaDefinitionUnless(
asserts == Nil || discrims == Nil,
"Cannot have both dfdl:discriminator annotations and dfdl:assert
annotations at the same location."
)
```
So it seems like we can't actually have assertions and discriminators on the
same element, so it's impossible for order to matter? We either get a single
discriminator or multiple asserts, but not both?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/Grammar.scala:
##########
@@ -83,14 +84,27 @@ class SeqComp private (context: SchemaComponent, children:
Seq[Gram])
protected final override def close = ")"
lazy val parserChildren =
- children.filter(_.forWhat != ForUnparser).map { _.parser }.filterNot {
- _.isInstanceOf[NadaParser]
- }
+ children
+ .filter(_.forWhat != ForUnparser)
+ .map { _.parser }
+ .filterNot {
+ _.isInstanceOf[NadaParser]
+ }
+ .toArray
+
+ lazy val assertExpressionChildren = parserChildren.filter {
Review Comment:
Can be removed also, the one parser this is passed into doesn't use this
anymore.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/Parser.scala:
##########
@@ -227,26 +227,25 @@ abstract class CombinatorParser(override val context:
RuntimeData)
extends Parser
with CombinatorProcessor
-final class SeqCompParser(context: RuntimeData, val childParsers:
Array[Parser])
- extends CombinatorParser(context) {
+final class SeqCompParser(
+ context: RuntimeData,
+ val childParsers: Array[Parser],
+ testAssert: Array[Parser]
+) extends CombinatorParser(context) {
override def runtimeDependencies = Vector()
override def childProcessors = childParsers.toVector
override def nom = "seq"
- val numChildParsers = childParsers.size
-
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
- i += 1
+ childParsers.foreach { cp =>
+ if (pstate.processorStatus.isSuccess) cp.parse1(pstate)
+ else cp match {
+ case ae: AssertExpressionEvaluationParser if (ae.discrim) =>
pstate.withTempSuccess(ae.parse1)
+ case _ => // processorStatus is Failure, do not execute non-discim
child parsers
+ }
Review Comment:
One potential issue with this approach is we don't short circuit failures.
For example, say there are 10000 parsers associated with this sequence and the
first one fails--we now have to iterate over all the remaining parsers and
ignore them just in case there is a discriminator at the end. Ideally on error
this would skip to the discriminator parser, evaluate that (assuming it hasn't
already been evaluated), and then end.
That said, considering I just realized that we don't even support asserts +
discrims on the same element right now, then order doesn't matter and your
previous approach is probably better since it does short circuit failures. I
think we can remove the latest fixup commit and we'll get the correct behavior
more efficiently.
We'll need careful changes here and tests if we ever do support asserts +
discrims on the same element, but I don't expect that to happen anytime soon.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/Grammar.scala:
##########
@@ -89,10 +90,19 @@ class SeqComp private (context: SchemaComponent, children:
Seq[Gram])
_.isInstanceOf[NadaParser]
}
+ lazy val assertExpressionChildren = parserChildren.filter {
+ _.isInstanceOf[AssertExpressionEvaluationParser]
Review Comment:
I think I've discovered that we don't even support asserts + discrims on the
same element. I agree this is probably the right behavior to use schema order,
but until we support asserts + discrims (if ever) I don't think we need to
consider this case.
--
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]
