mbeckerle commented on code in PR #1218:
URL: https://github.com/apache/daffodil/pull/1218#discussion_r1569310707
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/UnseparatedSequenceUnparsers.scala:
##########
@@ -107,78 +106,58 @@ class OrderedUnseparatedSequenceUnparser(
// startArray event. If we don't then
// the element must be entirely optional, so we get no events for it
// at all.
- //
- if (state.inspect) {
- val ev = state.inspectAccessor
+
+ // we must have an event
+ Assert.invariant(state.inspect, "No event for unparing.")
+
+ val ev = state.inspectAccessor
+ if (ev.erd eq erd) {
+ // must be a start event for this array/optional unparser
val isArr = ev.isArray
- if (ev.isStart && (isArr || ev.erd.isOptional)) {
- val eventNQN = ev.namedQName
- if (ev.erd eq erd) {
- //
- // StartArray for this unparser's array element
- //
- unparser.startArrayOrOptional(state)
- while ({
- doUnparser = unparser.shouldDoUnparser(unparser, state)
- doUnparser
- }) {
- if (isArr)
- if (state.dataProc.isDefined)
- state.dataProc.get.beforeRepetition(state, this)
-
- unparseOne(unparser, erd, state)
- numOccurrences += 1
- state.moveOverOneArrayIterationIndexOnly()
- state.moveOverOneOccursIndexOnly()
- state.moveOverOneGroupIndexOnly() // array elements are
always represented.
-
- if (isArr)
- if (state.dataProc.isDefined)
- state.dataProc.get.afterRepetition(state, this)
- }
-
- unparser.checkFinalOccursCountBetweenMinAndMaxOccurs(
- state,
- unparser,
- numOccurrences,
- maxReps,
- state.arrayIterationPos - 1,
- )
- unparser.endArrayOrOptional(erd, state)
- } else {
- //
- // start array but not for the expected array,
- // rather for some other array. Not this one. So we
- // don't unparse anything here, and we'll go on to the next
- // sequence child, which hopefully will be a matching array.
- //
- // minReps has to be 0, meaning it is allowed to have zero
- // occurrences (not necessarily valid, but allowed),
- // because we got zero instances of this array
- //
- Assert.invariant(minReps == 0L)
- }
- } else if (ev.isStart) {
- Assert.invariant(!isArr && !ev.erd.isOptional)
- //
- // start of scalar.
- // That has to be for a different element later in the sequence
- // since this one has a RepUnparser (i.e., is NOT scalar)
- val eventNQN = ev.namedQName
- Assert.invariant(eventNQN != erd.namedQName)
- } else {
- Assert.invariant(ev.isEnd && ev.erd.isComplexType)
- unparser.checkFinalOccursCountBetweenMinAndMaxOccurs(
- state,
- unparser,
- numOccurrences,
- maxReps,
- 0,
- )
+ Assert.invariant(ev.isStart && (isArr || ev.erd.isOptional))
+
+ //
+ // StartArray for this unparser's array element
+ //
+ unparser.startArrayOrOptional(state)
+ while ({
+ doUnparser = unparser.shouldDoUnparser(unparser, state)
+ doUnparser
+ }) {
+ if (isArr)
+ if (state.dataProc.isDefined)
+ state.dataProc.get.beforeRepetition(state, this)
+
+ unparseOne(unparser, erd, state)
+ numOccurrences += 1
+ state.moveOverOneArrayIterationIndexOnly()
+ state.moveOverOneOccursIndexOnly()
+ state.moveOverOneGroupIndexOnly() // array elements are always
represented.
+
+ if (isArr)
+ if (state.dataProc.isDefined)
+ state.dataProc.get.afterRepetition(state, this)
}
+
+ unparser.checkFinalOccursCountBetweenMinAndMaxOccurs(
+ state,
+ unparser,
+ numOccurrences,
+ maxReps,
+ state.arrayIterationPos - 1,
+ )
+ unparser.endArrayOrOptional(erd, state)
} else {
- // no event (state.inspect returned false)
- Assert.invariantFailed("No event for unparing.")
+ // this is either a start event for a following element or an end
event for a
+ // parent element. Either way, we never saw a start event for this
array/optional,
+ // which means there were zero occurrenes. Make sure that is valid
for this array
Review Comment:
The concept of 'valid' here vs. 'well formed' gives me some concern.
When dfdl:occursCountKind='parsed', then any number of occurrences,
including zero, is 'well formed', just not valid.
Further, when unparsing, if the number of occurrences is < minOccurs we're
supposed to check if the element is defaultable (rules are quite complex for
this, esp. for an element of complex type which is why this is not yet
implemented I believe.)
I believe the change here is an improvement to the code (avoids an abort at
least).
I'm just trying to make sure the comment isn't misleading in this quite
complex area where we are knowingly partial in what we've implemented of DFDL
defaulting behavior.
Maybe just change the "Make sure that is valid for this array" to "Make sure
that is **_allowed_** for this array" ?
I also feel like we should somehow comment this to be revisited when
defaulting during unparsing is considered.
--
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]
