stevedlawrence commented on code in PR #987:
URL: https://github.com/apache/daffodil/pull/987#discussion_r1904294992
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/PState.scala:
##########
@@ -588,6 +589,23 @@ final class PState private (
}
}
+ /**
+ * This function is used for cases where a parse must be performed after
there
+ * has already been a failed parse of an enclosing element/sequence. Most
+ * common example of this would be a choice branch containing a sequence with
+ * an annotated assert expression. According to 9.5.2 of the DFDL spec this
+ * assert expression needs to be parsed regardless of whether or not the
+ * enclosing sequence content parses or not as the assert expression may be
+ * used as a discriminator for the choice branch.
+ */
+ def withTempSuccess(func: (PState) => Unit): Unit = {
+ val priorProcessorStatus = processorStatus
+ setSuccess()
+ func(this)
+ if (processorStatus eq Success)
+ _processorStatus = priorProcessorStatus
Review Comment:
Conditionally resetting back to the previous status feels like it might be
confusing? Feels like if this is temporarily ignoring status it should always
reset back to whatever the status was before. Maybe the way this wants to work
is it it returns the status of `func`, and the caller can choose to do with it
whatever they want? Something more like
```scala
def withTempSuccess(func: (PState) => Unit): ProcessorResult = {
val priorStatus = processorStatus
setSuccess()
func(this)
val funcStatus = processorStatus
_processorStatus = priorStatus
funcStatus
}
```
Also, Is there any value in passing in a lambda? An alternative would be to
just pass in a `Parser`, and then instead of calling `func(this)`, it could
call `parser.parse1(this)`. That ensures that this is always called with a
`Parser` instead of a function that just happens to accept a `PState`, which is
probably safer?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/DFDLStatementMixin.scala:
##########
@@ -217,7 +217,7 @@ trait ProvidesDFDLStatementMixin extends ThrowsSDE with
HasTermCheck {
final lazy val patternStatements: Seq[DFDLStatement] = patternAsserts ++
patternDiscrims
final lazy val lowPriorityStatements: Seq[DFDLStatement] =
Review Comment:
Any idea where "low priority" comes from? I'm not familiar with that as a
DFDL term. Wondering if something like "expressionStatements" would be more
clear?
##########
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]
+ }
+
final override lazy val parser = {
if (parserChildren.isEmpty) new NadaParser(context.runtimeData)
else if (parserChildren.length == 1) parserChildren.head
- else new SeqCompParser(context.runtimeData, parserChildren.toVector)
+ else
+ new SeqCompParser(
+ context.runtimeData,
+ parserChildren.toVector,
+ assertExpressionChildren.toVector,
Review Comment:
I believe the `paserChildren` vector contains the `assertExpressionChildren`
parsers, which I think means the assert parsers will be evaluated twice? Once
when all the `parserChildren` are evaluated and again after the parserChildren
finis and then we evaluate the asserts Parsers? Seems like parserChildren
should not contain the assert children?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/Parser.scala:
##########
@@ -241,8 +244,14 @@ final class SeqCompParser(context: RuntimeData, val
childParsers: Vector[Parser]
while (i < numChildParsers) {
val parser = childParsers(i)
parser.parse1(pstate)
- if (pstate.processorStatus ne Success)
+ if (pstate.processorStatus ne Success) {
+ if (testAssert.nonEmpty) {
+ testAssert.foreach { ap =>
+ pstate.withTempSuccess(ap.parse1)
+ }
+ }
Review Comment:
Ah, I guess I now see why it's okay for `childrenParsers` to also include
the assertion parsers. If all the non-assert parsers succeed then we'll don't
evaluate the assertion parsers.
However, I think there is still an issue. Say all the non-assertion parsers
succeed, then we start evaluating the assertion parsers, and assume one of them
fails. The `pstate.processorStatus ne Success` will be true, and then we'll
evaluate all the assertion parsers, including ones we've already evaluated.
So I think this approach still feels like it needs a tweak. Almost feels
like the assert parsers need to be completely separate from the other parsers
or something.
--
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]
