tuxji commented on a change in pull request #429:
URL: https://github.com/apache/incubator-daffodil/pull/429#discussion_r500594335
##########
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/SequenceCombinator.scala
##########
@@ -45,8 +47,27 @@ abstract class SequenceCombinator(sq: SequenceTermBase,
sequenceChildren: Seq[Se
class OrderedSequence(sq: SequenceTermBase, sequenceChildrenArg:
Seq[SequenceChild])
extends SequenceCombinator(sq, sequenceChildrenArg) {
+ private lazy val sepMtaGram = sq.sequenceSeparatorMTA
+ // Note that we actually only every use one of these depending on depending
+ // on various factors. If there is an optional separtor and a suspension is
+ // used to unaprse that separator, then we cannot use the sepMtaUnaprser
+ // because it results in nested suspensions, which isn't allowed. In that
+ // case, the suspension ends up handling both the optional separator and
+ // alignment using sepMtaAlignmentMaybe.
+ private lazy val (sepMtaAlignmentMaybe, sepMtaUnparserMaybe) =
Review comment:
Reads better as:
```
// Note that we actually only ever use one of these depending on
// various factors. If there is an optional separator and a suspension is
// used to unparse that separator, then we cannot use the sepMtaUnparser
// because it results in nested suspensions, which isn't allowed. In that
// case, the suspension ends up handling both the optional separator and
// alignment using sepMtaAlignmentMaybe.
```
##########
File path:
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd
##########
@@ -400,6 +400,32 @@
</xs:documentation>
</xs:annotation>
</xs:element>
+ <xs:element name="unparseSuspensionWaitOld" type="xs:int"
default="100" minOccurs="0">
+ <xs:annotation>
+ <xs:documentation>
+ While unparsing, some unparse actions require "suspending" which
+ requires buffering unparse output until the suspension can be
+ evaluated. Daffodil periodically attempts to reevaluate these
+ suspensions so that these buffers can be released. We attempt to
+ evaluate young suspensions shortly after creation with the hope
+ that it will succeed and we can release associated buffers. But
if
+ a young suspension fails it is moved to the old suspension list.
+ Old suspensions are evaluated less frequently since they are less
+ likely to succeeded. This minimizes the overhead related to
+ evaluating suspensions that are likely to fail. The
+ unparseSuspensionWaitYoung and unparseSuspensionWaitOld
+ values determine how many elements unparsed before evaluating
Review comment:
Reads better as
`values determine how many elements are unparsed before evaluating`
##########
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/SequenceCombinator.scala
##########
@@ -98,8 +123,27 @@ class UnorderedSequence(sq: SequenceTermBase,
sequenceChildrenArg: Seq[SequenceC
import SeparatedSequenceChildBehavior._
+ private lazy val sepMtaGram = sq.delimMTA
+ // Note that we actually only ever use one of these depending on depending
+ // on various factors. If there is an optional separtor and a suspension is
+ // used to unaprse that separator, then we cannot use the sepMtaUnaprser
+ // because it results in nested suspensions, which isn't allowed. In that
+ // case, the suspension ends up handling both the optional separator and
+ // alignment using sepMtaAlignmentMaybe.
+ private lazy val (sepMtaAlignmentMaybe, sepMtaUnparserMaybe) =
Review comment:
Same here, reads better as:
```
// Note that we actually only ever use one of these depending on
// various factors. If there is an optional separator and a suspension is
// used to unparse that separator, then we cannot use the sepMtaUnparser
// because it results in nested suspensions, which isn't allowed. In that
// case, the suspension ends up handling both the optional separator and
// alignment using sepMtaAlignmentMaybe.
```
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/SeparatedSequenceUnparsers.scala
##########
@@ -182,10 +212,15 @@ class OrderedSeparatedSequenceUnparser(
trailingSuspendedOps:
Buffer[SuppressableSeparatorUnparserSuspendableOperation],
onlySeparatorFlag: Boolean): Unit = {
val doUnparseChild = !onlySeparatorFlag
- // We don't know if the unparse will result in zero length or not.
- // We have to use a suspendable unparser here for the separator
- // which suspends until it is known whether the unparse of the contents
- // were ZL or not.
+ // We don't know if the unparse will result in zero length or not. We have
+ // to use a suspendable unparser here for the separator which suspends
+ // until it is known whether the unparse of the contents were ZL or not. If
+ // the suspension determines that the field is non-zero length then the
+ // suspenion must also unparser mandatory text alignment for the separator.
+ // This cannot be done with a standard MTA alignment unparser since that is
+ // a suspension and suspensions cannot create suspensions. This this
+ // suspension is also responsible for unparsing alignment if the separator
+ // should be unparsed.
Review comment:
Reads better as
```
// We don't know if the unparse will result in zero length or not. We
have
// to use a suspendable unparser here for the separator which suspends
// until it is known whether the unparse of the contents were ZL or not.
If
// the suspension determines that the field is non-zero length then the
// suspension must also unparse mandatory text alignment for the
separator.
// This cannot be done with a standard MTA alignment unparser since that
is
// a suspension and suspensions cannot create suspensions. This
suspension
// is also responsible for unparsing alignment if the separator should be
// unparsed.
```
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/SeparatedSequenceUnparsers.scala
##########
@@ -137,31 +142,56 @@ class OrderedSeparatedSequenceUnparser(
}
/**
- * Unparses the separator only.
+ * Unparses just the separator, as well as any mandatory text alignment if
it necessary
Review comment:
Reads better: `if necessary`
##########
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/SequenceCombinator.scala
##########
@@ -98,8 +123,27 @@ class UnorderedSequence(sq: SequenceTermBase,
sequenceChildrenArg: Seq[SequenceC
import SeparatedSequenceChildBehavior._
+ private lazy val sepMtaGram = sq.delimMTA
+ // Note that we actually only every use one of these depending on depending
+ // on various factors. If there is an optional separtor and a suspension is
+ // used to unaprse that separator, then we cannot use the sepMtaUnaprser
+ // because it results in nested suspensions, which isn't allowed. In that
+ // case, the suspension ends up handling both the optional separator and
+ // alignment using sepMtaAlignmentMaybe.
+ private lazy val (sepMtaAlignmentMaybe, sepMtaUnparserMaybe) =
+ if (sepMtaGram.isEmpty) {
+ (MaybeInt.Nope, Maybe.Nope)
+ } else {
+ (MaybeInt(sq.knownEncodingAlignmentInBits), Maybe(sepMtaGram.unparser))
Review comment:
Funny, codecov check seems to say 100% of diff is covered.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
