You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "mbeckerle (via GitHub)" <gi...@apache.org> on 2023/05/25 23:01:44 UTC
[GitHub] [daffodil] mbeckerle commented on a diff in pull request #1015: Fix separatorSuppressionPolicy="never"
mbeckerle commented on code in PR #1015:
URL: https://github.com/apache/daffodil/pull/1015#discussion_r1206038854
##########
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:
I don't see how isArr could be false. I think we're in a context here where it has to be true.
I believe we're still in the `case unparser: RepOrderedSeparatedSequenceChildUnparser => `
##########
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:
Right here I'd insert an `Assert.invariant(erd.isArray || erd.isOptional)` because it is the fact that the child unparser is a RepOrderedSeparatedSequenceChildUnparser that tells us for sure this is an array/optional. I am a bit uncertain about the isOptional test. It may be that it must be an array full stop.
##########
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:
is Positional still used? If not eliminate it. If it is still used we need a comment about what it represents because the others seem to cover all the bases.
##########
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:
We should move this up to where the erd is first made accessible.
##########
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:
Codecov warning here suggests this should just be an prototype method without a default implementation that does nothing.
Otherwise I think we should create a test to exercise this. It's going to be for some non-positional dfdl:separatorSuppressionPolicy='anyEmpty' case I think.
##########
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:
You added PositionalNever, but I can't see anywhere that it isn't treated as equivalent to Positional, which is what the value used to be. Can PositionalNever be eliminated again?
##########
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:
We really need to fix the cross-tester and grow an ability to see if these tests work on IBM. We have never actually made any of our test suite run against IBM DFDL. Just specific schemas like EDIFACT.
That goes beyond this change set though.
##########
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:
Hmmm. This suggests these tests were run against IBM DFDL to verify portability..... I am not sure we have actually done that except way back I attempted this, but this is years ago. and there were several hundred tests that failed, mostly due to comparison logic not taking type into consideration.
That work all predated our C-codegen back-end and the TDML refactoring to allow tests to run on the original backend or the C backend.
Maybe we can make it so if found the IBM test rig would dynamically load and then tests marked for ibm would run on IBM, but not otherwise.
##########
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)
+ if (state.dataProc.isDefined)
+ state.dataProc.get.beforeRepetition(state, this)
+
+ unparseOne(unparser, erd, state)
+ numOccurrences += 1
+ Assert.invariant(
+ state.arrayIterationIndexStack.length == arrayIterationIndexStackDepthBefore,
+ )
+ state.moveOverOneArrayIterationIndexOnly()
+ Assert.invariant(state.arrayIterationPos == arrayIterationIndexBefore + 1)
- if (isArr)
- if (state.dataProc.isDefined)
- state.dataProc.get.beforeRepetition(state, this)
+ Assert.invariant(state.occursIndexStack.length == occursIndexStackDepthBefore)
+ state.moveOverOneOccursIndexOnly()
+ Assert.invariant(state.occursPos == occursIndexBefore + 1)
- unparseOne(unparser, erd, state)
- numOccurrences += 1
- Assert.invariant(
- state.arrayIterationIndexStack.length == arrayIterationIndexStackDepthBefore,
- )
- state.moveOverOneArrayIterationIndexOnly()
- Assert.invariant(state.arrayIterationPos == arrayIterationIndexBefore + 1)
+ Assert.invariant(state.groupIndexStack.length == groupIndexStackDepthBefore)
+ state.moveOverOneGroupIndexOnly() // array elements are always represented.
+ Assert.invariant(state.groupPos == groupIndexBefore + 1)
- Assert.invariant(state.occursIndexStack.length == occursIndexStackDepthBefore)
- state.moveOverOneOccursIndexOnly()
- Assert.invariant(state.occursPos == occursIndexBefore + 1)
-
- Assert.invariant(state.groupIndexStack.length == groupIndexStackDepthBefore)
- state.moveOverOneGroupIndexOnly() // array elements are always represented.
- Assert.invariant(state.groupPos == groupIndexBefore + 1)
+ if (isArr)
Review Comment:
Again I think isArr is guaranteed true.
##########
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:
Comment is unclear. Why wouldn't this also be true of elements within the array?
I get how the first time the ev.erd matches the erd (when it did not before) that we are transitioning to a new array, but we're not keeping track of whether this is the first time or not here are we?
##########
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:
If instead of removing the if-then and adding the Assert.invariant, you could have done:
```
if (!state.inspect) Assert.invariantFailed(....)
else {
```
Then all the code wouldn't be re-indented and the diff would be more meaningful.
Perhaps put this back in that form, and change it late in the process to just the Assert.invariant.
Or maybe just putting in a extra set of block "{ .... }" would cause all the indentation to remain as it was.
##########
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:
Ok. I found it eventually. There appears to be exactly one place where this is needed.
--
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