You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "stevedlawrence (via GitHub)" <gi...@apache.org> on 2023/05/26 14:01:30 UTC
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #1015: Fix separatorSuppressionPolicy="never"
stevedlawrence commented on code in PR #1015:
URL: https://github.com/apache/daffodil/pull/1015#discussion_r1206728978
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -94,7 +95,30 @@ trait SeparatedSequenceChildParseResultHelper extends SequenceChildParseResultHe
* Define this as final here so we aren't creating proliferation of
* traits/classes just for this one little issue.
Review Comment:
I just copied this from the old `finalChecks` function--this follows the exact same pattern just called in a different place.
I think the idea is that things that implement this `SeparatedSequenceChildParseResultHelper` never have to think about implementing slightly different variants of this function, which could potentially lead to different traits with slightly different implementations?
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
Review Comment:
Personally I'd rather remove unnecessary indentation. It sometimes makes diffs a bit uglier but I think cleaner code is worth it. Also, if you hide diff whitespace by clicking the above cog, I think it makes the diff a bit more readable and shows that the changes here are more rasonable, and it more clearly shows which else- blocks have been removed (which is actually all of them).
Here's a screen shot of where to cog is--it's kindof a hidden feature:
> 
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
Review Comment:
I added `Assert.invariant(erd.isArray)` and multiple tests failed, so I can confirm that `RepOrderedSeparatedSequenceChildUnparser` is used for both arrays and optionals.
The suggested assertions works as expected.
##########
daffodil-test/src/test/scala/org/apache/daffodil/usertests/TestSepTests.scala:
##########
@@ -47,28 +47,18 @@ class TestSepTests {
runner.runOneTest("test_sep_trailingEmptyStrict_2")
}
- // DAFFODIL-2499 - separatorSuppressionPolicy 'never'
- // Note: this test isn't commented out, because it works for IBM DFDL in cross testing
- // The TDML for this test just has it disabled for the daffodil implementation.
- // Add daffodil to implementations to see the erroneous daffodil behavior.
Review Comment:
Yeah, we need some kind of solution for this. I think it's made a bit complicated because IBM DFDL and Daffodil have conflicting dependencies (I think it's just ICU at the moment). So we need something like OSGI, or special classpath loaders or something so that running Daffodil vs IBM DFDL uses the correct classpath. It's also made harder since sbt doesn't make it easy to change classpaths.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
+ val ev = state.inspectAccessor
+ val isArr = erd.isArray
- if (state.inspect) {
- val ev = state.inspectAccessor
- val isArr = erd.isArray
- if (ev.isStart && (isArr || erd.isOptional)) {
- if (ev.erd eq erd) {
- //
- // StartArray for this unparser's array element
- //
- unparser.startArrayOrOptional(state)
- while ({
- doUnparser = unparser.shouldDoUnparser(unparser, state)
- doUnparser
- }) {
- //
- // These are so we can check invariants on these stacks being
- // pushed and popped reliably, and incremented only once
- //
- val arrayIterationIndexBefore = state.arrayIterationPos
- val arrayIterationIndexStackDepthBefore =
- state.arrayIterationIndexStack.length
- val occursIndexBefore = state.occursPos
- val occursIndexStackDepthBefore = state.occursIndexStack.length
- val groupIndexBefore = state.groupPos
- val groupIndexStackDepthBefore = state.groupIndexStack.length
+ // If the event is for this Rep unparser, we need to consume the StartArray event
+ if (ev.erd eq erd) {
Review Comment:
This check is outside the while-loop that consume the actual elements.
So we first consume the StartArray event (and do nothing for optionals) and then do the while-loop that parses all the StartElement/EndElement events. Once the while-loop ends, we unparse any missing separators, and then consume the the EndArray event.
If there are zero occurrences of this rep unparser, then we won't consume any events, and all we'll do is unparse all the needed separators.
I think looking at the code itself or with diff whitespace hidden makes this a bit more clear.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -62,6 +62,7 @@ object SeparatedSequenceChildBehavior {
sealed abstract class PositionalLike extends Type
sealed abstract class PositionalTrailing extends PositionalLike
case object Positional extends PositionalLike
Review Comment:
It is still used. Honestly, I don't really understand what it represents, but a number of tests fail if it's not used. We do seem to need this distinction between Positional and PositionalNever. If you have any idea what it represents I can add a comment.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceChildParseResultHelper.scala:
##########
@@ -87,10 +87,22 @@ trait SequenceChildParseResultHelper extends Serializable {
requiredOptional: RequiredOptionalStatus,
): ParseAttemptStatus
+ /**
+ * Overridden for Positional case.
+ */
+ def arrayCompleteChecks(
+ parser: SequenceChildParser,
+ pstate: PState,
+ resultOfTry: ParseAttemptStatus,
+ priorResultOfTry: ParseAttemptStatus,
+ ): Unit = {
Review Comment:
I tracked this down, and this is just dead code that is impossible to hit.
The only `SequenceChildParserResultHelpers` implementations that need the `arrayCompleteChecks` function are the ones where the corresponding implementation of `SequenceChildParser` overrides `arrayCompleteChecks` to call `helper.arrayCompleteChecks(...)`. Not all `SequenceChildParsers` need a helper (at least according to the interface), so not all helpers need the function.
The same issue exists for the old `finalChecks` function (renamed to `sequenceCompleteChecks` in this PR). Both implementations can be removed.
##########
daffodil-test/src/test/resources/org/apache/daffodil/usertests/SepTests.tdml:
##########
@@ -223,7 +223,7 @@
<!-- Test for DAFFODIL-2499. empty strings should not be creating empty elements here. -->
<tdml:parserTestCase name="test_sep_ssp_never_1" root="file1" model="s3"
- implementations="ibm">
+ implementations="daffodil ibm">
<!--
This test doesn't work on Daffodil due to DAFFODIL-2499.
Review Comment:
Done
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
+ val ev = state.inspectAccessor
+ val isArr = erd.isArray
- if (state.inspect) {
- val ev = state.inspectAccessor
- val isArr = erd.isArray
- if (ev.isStart && (isArr || erd.isOptional)) {
- if (ev.erd eq erd) {
- //
- // StartArray for this unparser's array element
- //
- unparser.startArrayOrOptional(state)
- while ({
- doUnparser = unparser.shouldDoUnparser(unparser, state)
- doUnparser
- }) {
- //
- // These are so we can check invariants on these stacks being
- // pushed and popped reliably, and incremented only once
- //
- val arrayIterationIndexBefore = state.arrayIterationPos
- val arrayIterationIndexStackDepthBefore =
- state.arrayIterationIndexStack.length
- val occursIndexBefore = state.occursPos
- val occursIndexStackDepthBefore = state.occursIndexStack.length
- val groupIndexBefore = state.groupPos
- val groupIndexStackDepthBefore = state.groupIndexStack.length
+ // If the event is for this Rep unparser, we need to consume the StartArray event
+ if (ev.erd eq erd) {
+ unparser.startArrayOrOptional(state)
+ }
- Assert.invariant(
- erd.isRepresented,
- ) // since this is an array, can't have inputValueCalc
+ // Unparse each occurrence of this array in the infoset. Note that there could be zero
+ // occurrences
+ while ({
+ doUnparser = unparser.shouldDoUnparser(unparser, state)
+ doUnparser
+ }) {
+ //
+ // These are so we can check invariants on these stacks being
+ // pushed and popped reliably, and incremented only once
+ //
+ val arrayIterationIndexBefore = state.arrayIterationPos
+ val arrayIterationIndexStackDepthBefore =
+ state.arrayIterationIndexStack.length
+ val occursIndexBefore = state.occursPos
+ val occursIndexStackDepthBefore = state.occursIndexStack.length
+ val groupIndexBefore = state.groupPos
+ val groupIndexStackDepthBefore = state.groupIndexStack.length
+
+ Assert.invariant(
Review Comment:
Done
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/SequenceChild.scala:
##########
@@ -465,7 +465,7 @@ class ScalarOrderedSequenceChild(sq: SequenceTermBase, term: Term, groupIndex: I
isModelGroupRepPossiblyZeroLength,
isModelGroupRepNonZeroLength,
)
- case Positional =>
+ case Positional | PositionalNever =>
Review Comment:
There are a few cases where we do still assign `Positional`:
https://github.com/apache/daffodil/blob/main/daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/SequenceChild.scala#L131-L159
Those cases do not use the `PositionalNever` logic. I originally did not have this new `PositionalNever` thing and applied this check for all `Positional` children, but it broke a large number of tests. I admit I couldn't entirely follow the logic as to why, but I tried to add tests to make convince myself it was working as expected. We may need more tests to cover cases I haden't considered, if you have any thoughts let me know.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
+ val ev = state.inspectAccessor
+ val isArr = erd.isArray
- if (state.inspect) {
- val ev = state.inspectAccessor
- val isArr = erd.isArray
- if (ev.isStart && (isArr || erd.isOptional)) {
- if (ev.erd eq erd) {
- //
- // StartArray for this unparser's array element
- //
- unparser.startArrayOrOptional(state)
- while ({
- doUnparser = unparser.shouldDoUnparser(unparser, state)
- doUnparser
- }) {
- //
- // These are so we can check invariants on these stacks being
- // pushed and popped reliably, and incremented only once
- //
- val arrayIterationIndexBefore = state.arrayIterationPos
- val arrayIterationIndexStackDepthBefore =
- state.arrayIterationIndexStack.length
- val occursIndexBefore = state.occursPos
- val occursIndexStackDepthBefore = state.occursIndexStack.length
- val groupIndexBefore = state.groupPos
- val groupIndexStackDepthBefore = state.groupIndexStack.length
+ // If the event is for this Rep unparser, we need to consume the StartArray event
+ if (ev.erd eq erd) {
+ unparser.startArrayOrOptional(state)
+ }
- Assert.invariant(
- erd.isRepresented,
- ) // since this is an array, can't have inputValueCalc
+ // Unparse each occurrence of this array in the infoset. Note that there could be zero
+ // occurrences
+ while ({
+ doUnparser = unparser.shouldDoUnparser(unparser, state)
+ doUnparser
+ }) {
+ //
+ // These are so we can check invariants on these stacks being
+ // pushed and popped reliably, and incremented only once
+ //
+ val arrayIterationIndexBefore = state.arrayIterationPos
+ val arrayIterationIndexStackDepthBefore =
+ state.arrayIterationIndexStack.length
+ val occursIndexBefore = state.occursPos
+ val occursIndexStackDepthBefore = state.occursIndexStack.length
+ val groupIndexBefore = state.groupPos
+ val groupIndexStackDepthBefore = state.groupIndexStack.length
+
+ Assert.invariant(
+ erd.isRepresented,
+ ) // since this is an array, can't have inputValueCalc
+
+ if (isArr)
Review Comment:
These rep unparsers are used for both arrays and optionals. If isOptional is true, then isArray is always false.
##########
daffodil-test/src/test/resources/org/apache/daffodil/usertests/SepTests.tdml:
##########
@@ -268,7 +268,7 @@
<!-- Test for DAFFODIL-2499. empty strings should not be creating empty elements here. -->
<tdml:parserTestCase name="test_sep_ssp_never_3" root="file1" model="s3"
- implementations="ibm">
+ implementations="daffodil ibm">
Review Comment:
Agreed. Might be a pretty large effort though.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org