stevedlawrence commented on code in PR #1680:
URL: https://github.com/apache/daffodil/pull/1680#discussion_r3397886461
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedParseHelper.scala:
##########
@@ -227,8 +226,13 @@ final class PostfixSeparatorHelper(
// usually (except first one in infix case) don't run the child
parser
val failure = pstate.processorStatus.asInstanceOf[Failure]
- Assert.invariant(!pstate.isPointOfUncertaintyResolved(pou))
- pstate.resetToPointOfUncertainty(pou)
+ if (!pstate.isPointOfUncertaintyResolved(pou)) {
+ pstate.resetToPointOfUncertainty(pou)
+ } else {
+ // pou was resolved by a discriminator somewhere, so
+ // we need to resolve the outer POU here
+ pstate.resolvePointOfUncertainty()
Review Comment:
This doesn't feel right to me. I think the only thing that should resolve
PoUs are things like discriminators, initiated content, etc.
It's not even clear to me why we need a PoU here. Similarly
InfixPrefixSeparatorHelperMixin has this:
```scala
pstate.withPointOfUncertainty(
"parseOneWithInfixOrPrefixSeparator",
childParser.context
) { pou =>
sep.parse1(pstate)
val rv = if (pstate.processorStatus eq Success) {
SeparatorParseStatus.SeparatorFound
} else {
// reset to the point of uncertainty to discard the errors
pstate.resetToPointOfUncertainty(pou)
SeparatorParseStatus.SeparatorFailed
}
rv
}
```
That creates a PoU, but doesn't care if it was resolved or not, and doesn't
reset back to anything if it fails. Seems like this PoU isn't actually doing
anything.
I wonder if all these things are actually relying on an outter separate, and
this inner was is actually incorrect to have? The `parseOneInstance` already
has the logic to determine whether or not a PoU is needed and then creates one,
and then has logic based on if the PoU was resolved or not. Is that really the
only PoU we need for all these separated things?
And the fact that you added a change to resolve the outer PoU maybe
indicates this is the case? If this PoU weren't created, then the discriminator
would resolve the outer PoU and we shouldn't need this logic.
That said, there is code below where we do reset back to the beginning of
this PoU so we do need some PoU here? Is it possible that rather than creating
a new PoU this helpers should be getting passed in the outter PoU and then can
reset back to the beginning of it? And I wonder if we need an ability to reset
back to a PoU without discarding it, so that we can reset back to it again
multiple times?
--
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]
