stevedlawrence commented on code in PR #981:
URL: https://github.com/apache/daffodil/pull/981#discussion_r1129820680
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/LocalElementMixin.scala:
##########
@@ -84,6 +85,10 @@ trait LocalElementMixin extends ParticleMixin with
LocalElementGrammarMixin {
else if (
isDefaultable && !hasTerminator && emptyValueDelimiterPolicy =:=
EmptyValueDelimiterPolicy.Terminator
) true
+ else if (
+ hasInitiator && hasTerminator && emptyValueDelimiterPolicy =:=
EmptyValueDelimiterPolicy.None
Review Comment:
I'm not sure I understand this? Seems having an initiator/terminator
shouldn't matter if `emptyValueDelimiterPolicy="none"`?
I wonder if the `isDefaultable && emptyValueDelimiterPolicy =:=
EmptyValueDelimiterPolicy.None` check is incorrect? I'm not sure why
isDefaultable matters when determining if something could be missing. Or maybe
a check in `isDefaultable` is incorrect?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/PrimitivesDelimiters.scala:
##########
@@ -68,15 +69,33 @@ abstract class DelimiterText(
case _ => false
}
+ // When the value is known to be empty, is this delimiter required to
+ // be present according to the emptyValueDelimiterPolicy
+ private val requiredOnEmptyValue = e match {
+ case elemB: ElementBase =>
elemB.isDelimiterRequiredWhenEmpty(delimiterType)
+ case _ => true
+ }
+
+ // When the value is not yet known to be empty, can we continue parsing
+ // if we have a missing delimiter
+ private val proceedOnMissingDelimiter = e match {
+ case elemB: ElementBase =>
+ delimiterType == DelimiterTextType.Initiator &&
elemB.emptyValueDelimiterPolicy == EmptyValueDelimiterPolicy.Terminator
Review Comment:
What about the `none` policy? Doesn't that mean the initiator is optional
and we can proceed if it's missing?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/DelimiterParsers.scala:
##########
@@ -108,12 +109,14 @@ class DelimiterTextParser(
if (foundDelimiter.isDefined) {
if (!containsLocalMatch(foundDelimiter.get.matchedDFAs, start)) {
// It was a remote delimiter but we should have found a local one.
- PE(
- start,
- "Found out of scope delimiter: '%s' '%s'",
- foundDelimiter.get.matchedDFAs(0).lookingFor,
-
Misc.remapStringToVisibleGlyphs(foundDelimiter.get.matchedDelimiterValue.get),
- )
+ if (requiredOnEmptyValue) {
Review Comment:
I'm not sure I understand this. We found a delimiter, but not the one we
were looking for. Seems like that should always be a PE?
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/DelimiterUnparsers.scala:
##########
@@ -54,6 +55,15 @@ class DelimiterTextUnparser(
s"Unparsing starting at bit position:
${state.dataOutputStream.maybeAbsBitPos0b}",
)
+ // Do not emit the delimiter if the value is empty and the
emptyValueDelimiterPolicy
+ // indicates it should be suppressed
+ if (state.currentInfosetNode.isSimple) {
+ val node = state.currentInfosetNode.asSimple
+ if (node.hasValue && node.isEmpty && !requiredOnEmptyValue) {
+ return
Review Comment:
We usually try to avoid return. I think the only way around it is to wrap
everyhing in an if-block, which could be messy. So maybe it's fine in this
case? Thoughts from others?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/DelimiterParsers.scala:
##########
@@ -145,8 +147,8 @@ class DelimiterTextParser(
localTypedDelims = localTypedDelims + " " +
scannedDelims.next().lookingFor
}
- PE(start, "%s '%s' not found", delimiterType.toString, localTypedDelims)
- return
+ if (!proceedOnMissingDelimiter)
+ PE(start, "%s '%s' not found", delimiterType.toString,
localTypedDelims)
Review Comment:
Where do we require that the value was empty? It seems like this would allow
a delimiter to be missing when there is a non-empty value?
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/DelimiterUnparsers.scala:
##########
@@ -54,6 +55,15 @@ class DelimiterTextUnparser(
s"Unparsing starting at bit position:
${state.dataOutputStream.maybeAbsBitPos0b}",
)
+ // Do not emit the delimiter if the value is empty and the
emptyValueDelimiterPolicy
+ // indicates it should be suppressed
+ if (state.currentInfosetNode.isSimple) {
Review Comment:
Does emptyValueDelmiterPolicy not apply on complex elements? Can't complex
elements be empty as well?
--
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]
