stevedlawrence commented on code in PR #1652:
URL: https://github.com/apache/daffodil/pull/1652#discussion_r3372987445
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -254,45 +254,55 @@ trait ElementBaseGrammarMixin
final lazy val prefixedLengthBody = prefixedLengthElementDecl.parsedValue
def checkEndOfParentRestrictionsOnCurrentElement(optParentELU:
Option[LengthUnits]): Unit = {
+ Assert.invariant(optParentELU.isDefined)
Review Comment:
If we are asserting that optParentELU is defined, can we make this not an
Option and force callers to do any necessary checks?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -648,20 +658,20 @@ trait ElementBaseGrammarMixin
}
case LengthKind.Pattern =>
schemaDefinitionError("Binary data elements cannot have
lengthKind='pattern'.")
+ case LengthKind.EndOfParent if optionBinaryCalendarRep.isDefined =>
Review Comment:
I think it's possible to have `dfdl:binaryCalendarRep` defined but not be
applicable and so it would normally be ignored, e.g.
```xml
<element name="foo"
type="xs:int"
dfdl:lengthKind="endOfParent"
dfdl:binaryNumberRep="packed"
dfdl:binaryCalendarRep="binaryMilliseconds" ... />
```
The code will currently error because `binaryCalendarRep` is defined even
though it won't be used because the type is not a date/time/dateTime. I think
this wants to have logic similar to the delimited case where it checks the
primType and does different checks for calendar types vs non calendar types.
Same issue with optionBinaryNumberRep below
In fact, I think the EOP checks are exactly the same as the delimited
checks, so can we do something like this with a tweak to SDE to include the
right type in the diagnostic:
```scala
case LengthKind.Delimited | LengthKind.EndOfParent => {
...
SDE(
"lengthKind='%s' only supported for packed binary formats.",
repElement.lengthKind.toString
)
...
}
```
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -281,24 +281,25 @@ trait ElementBaseGrammarMixin
|| (currentElement.representation == Representation.Text)
|| (currentElement.primType eq PrimType.HexBinary)
|| (currentElement.representation == Representation.Binary
- && ((currentElement.binaryNumberRep match {
- case BinaryNumberRep.Packed => true
- case BinaryNumberRep.Bcd => true
- case BinaryNumberRep.Ibm4690Packed => true
+ && ((
+ currentElement.binaryNumberRep,
+ currentElement.optionBinaryCalendarRep
+ ) match {
+ case (
+ BinaryNumberRep.Packed | BinaryNumberRep.Bcd |
BinaryNumberRep.Ibm4690Packed,
+ _
+ ) =>
+ true
+ case (
+ _,
+ Some(
+ BinaryCalendarRep.Packed | BinaryCalendarRep.Bcd |
+ BinaryCalendarRep.Ibm4690Packed
+ )
+ ) =>
+ true
Review Comment:
scala-fmt makes this impossible to make sense of. Suggest we revert back to
the old way
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1341,18 +1451,29 @@ trait ElementBaseGrammarMixin
// non-explicit lengthKind
val body = bodyArg
+ val eopSimpleTypeElementThatNeedsBitLimit =
+ (isSimpleType && lengthKind == LengthKind.EndOfParent)
+ && !this.isInstanceOf[Root]
Review Comment:
Why do do we need to consider simple types and root elements? It feels like
the only thing that determines if we need the `SpecifiedLengthOfOnParentParser`
is if the `body` parser expects something else to set the parents length or if
it figures it out itself. The only things that do that are hexBinary and packed
binary parsers, so everything else needs it.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1341,18 +1451,29 @@ trait ElementBaseGrammarMixin
// non-explicit lengthKind
val body = bodyArg
+ val eopSimpleTypeElementThatNeedsBitLimit =
+ (isSimpleType && lengthKind == LengthKind.EndOfParent)
+ && !this.isInstanceOf[Root]
+ && impliedRepresentation != Representation.Text
+ && !isNillable
+ && primType != PrimType.HexBinary
// there are essentially two categories of processors that read/write data
input/output
- // stream: those that calculate lengths themselves and those that expect
another
- // processor to calculate the length and set the bit limit which this
processor will use as
- // the length. The following determines if this element requires another
processor to
- // calculate and set the bit limit, and if so adds the appropriate grammar
to do that
- val bodyRequiresSpecifiedLengthBitLimit = lengthKind !=
LengthKind.Delimited && (
- isSimpleType && impliedRepresentation == Representation.Text ||
- isSimpleType && isNillable ||
- isComplexType && lengthKind != LengthKind.Implicit ||
- lengthKind == LengthKind.Prefixed ||
- isSimpleType && primType == PrimType.HexBinary && lengthKind ==
LengthKind.Pattern
- )
+ // stream: those that calculate lengths themselves (ex: binary numeric
parsers) and those
+ // that expect another processor to calculate the length and set the bit
limit which
+ // this processor will use as the length (such as text parsers). The
following determines
+ // if this element requires another processor to calculate and set the bit
limit, and if so
+ // adds the appropriate grammar to do that
+ val bodyRequiresSpecifiedLengthBitLimit = lengthKind !=
LengthKind.Delimited
+ // Note for non-root EndOfParent simple types, we don't wish to duplicate
the length
+ // calculation efforts unless we know it needs the bit limit set by a
parent
+ && !eopSimpleTypeElementThatNeedsBitLimit
+ && (
+ isSimpleType && impliedRepresentation == Representation.Text ||
+ isSimpleType && isNillable ||
+ isComplexType && lengthKind != LengthKind.Implicit ||
+ lengthKind == LengthKind.Prefixed ||
+ isSimpleType && primType == PrimType.HexBinary && lengthKind ==
LengthKind.Pattern
+ )
Review Comment:
I'm finding it hard to make sense of all of these conditions and how they
combine with the new eop boolean. Would it simplify anything to have separate
match cases based on lengthKind, e.g.
```scala
val bodyRequiresSpecifiedLengthBitLimit = lengthKind match {
case LengthKind.Delimited => false
case LengthKind.EndOfParent => {
// is string or nillable or complex
}
case _ =>
// existing logic
}
```
This way we don't need to consider how EOP interacts with the existing
logic, which was all kindof designed without EOP in mind.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +95,149 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def optEffectiveLengthUnits(optLastNonEOPELU: Option[LengthUnits]):
Option[LengthUnits] = {
+ this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPELU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case LengthKind.Implicit | LengthKind.Delimited =>
+ None // invalid parent; SDE fires separately
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // Sequences are transparent — the ELU is inherited from the nearest
enclosing box.
+ case _: SequenceTermBase => optLastNonEOPELU
+ case _: ChoiceTermBase => None // implicit-length choice; SDE fires
separately
+ }
+ }
+
+ final lazy val childrenEndOfParent: Seq[ElementBase] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(lastNonEOPELU: Option[LengthUnits]):
Boolean = {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ lazy val optParentELU = term.optEffectiveLengthUnits(lastNonEOPELU)
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach { child =>
+ child.checkEndOfParentRestrictionsOnCurrentElement(lastNonEOPELU)
+ }
+ }
+ case e: ElementBase if eopChildren.nonEmpty => {
+ eopChildren.foreach { child =>
+ child.schemaDefinitionWhen(
+ e.lengthKind == LengthKind.Implicit || e.lengthKind ==
LengthKind.Delimited,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but its
parent is an element with dfdl:lengthKind 'implicit' or 'delimited'."
+ )
+ child.checkEndOfParentRestrictionsOnCurrentElement(optParentELU)
+ }
+ }
+ case s: SequenceTermBase if eopChildren.nonEmpty => {
+ eopChildren.foreach { child =>
+ child.schemaDefinitionWhen(
+ s.separatorPosition == SeparatorPosition.Postfix,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is
in a sequence with dfdl:separatorPosition defined as 'postfix'."
+ )
+ child.schemaDefinitionWhen(
+ s.sequenceKind != SequenceKind.Ordered,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is
in a sequence with dfdl:sequenceKind defined as 'unordered'."
+ )
+ child.schemaDefinitionWhen(
+ s.hasTerminator,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is
in a sequence with a dfdl:terminator."
+ )
+ child.schemaDefinitionWhen(
+ s.realElementChildren.exists(e => e.floating == YesNo.Yes),
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is
in a sequence with elements defining dfdl:floating='yes'."
+ )
+ child.schemaDefinitionWhen(
+ s.trailingSkip != 0,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is
in a sequence with a non-zero dfdl:trailingSkip."
+ )
+ }
+
+ }
+ case c: ChoiceTermBase if eopChildren.nonEmpty => {
+ // TODO: The DFDL spec (12.3.6) explicitly mentions that an
EndOfParent element
+ // can be terminated by a choice with choiceLengthKind='explicit',
with no reference
+ // to implicit length choices. Later on in 12.3.6, it specified what
the parent Effective
+ // Length Units would be for an explicit length choice, again with no
reference to
+ // implicit length choices. The only way to get an EndOfParent element
within an
+ // implicit length choice would be to have it actually be terminated
by the choice's
+ // parent, similar to how we treat elements within an element that is
also EndOfParent.
+ // The only mention of implicit length choices in the DFDL Spec is to
mention SDEs when
+ // an EndOfParent element is enclosed by a choice with
choiceLengthKind='implicit'. We
+ // think that may be a typo, so for now we disallow
ChoiceLengthKind.Implicit
+ // enclosing an EndOfParent element.
+ // See Daffodil-3080
+ eopChildren.foreach { child =>
+ child.schemaDefinitionWhen(
+ c.choiceLengthKind == ChoiceLengthKind.Implicit,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but its
parent is a choice with dfdl:choiceLengthKind 'implicit'."
+ )
+ child.schemaDefinitionWhen(
+ c.hasTerminator,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is
in a choice with a dfdl:terminator."
+ )
+ child.schemaDefinitionWhen(
+ c.trailingSkip != 0,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is
in a choice with a non-zero dfdl:trailingSkip."
+ )
+ child.checkEndOfParentRestrictionsOnCurrentElement(optParentELU)
+ }
+ }
+ case _ => // do nothing
+ }
+ // end checks
+ val sawEOP = term.termChildren.foldLeft(false) { case (sawEOP, child) =>
+ if (sawEOP) {
+ // Choice branches are alternatives, not sequential data — the
after-EOP SDE must
+ // not fire across branches. Only sequences and elements have
sequential ordering.
+ term match {
+ case _: ChoiceTermBase => // has alternatives which can all be EOP;
skip
+ case _ =>
+ child.schemaDefinitionWhen(
+ child.isInstanceOf[ModelGroup],
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but a
model group is defined between this element and the end of the enclosing
component"
Review Comment:
Note that the `child` context of the SDE in this case is the model group.
But the diagnostic makes it sound like it's the EOP element.
We could change the diagnostic to be something like "This model group is
defined after an an EOP in the containing model group". Could do something
similar with the below diagnostic.
Alternatively, instead of returning a sawEOP boolean, we could return a
`Maybe[Element]`. Where `Some(Element)` is the most recently seen EOP element,
and Nope means we haven't seen an EOP element. So insteead of `if (sawEOP)` we
do something like `if (optMostRecentEOPElement.isDefined)` (probably wants a
better name).
I dont' feel strongly either way.
##########
daffodil-core/src/main/scala/org/apache/daffodil/io/InputSource.scala:
##########
@@ -350,9 +356,19 @@ class BucketingInputSource(
bytesFilledInLastBucket = 0
lastBucketIndex += 1
if ((lastBucketIndex - oldestBucketIndex) >=
maxNumberOfNonNullBuckets) {
- // This frees the oldest bucket, allowing it to be garbage
collected.
- buckets(oldestBucketIndex) = null
- oldestBucketIndex += 1
+ if (buckets(oldestBucketIndex).refCount == 0) {
Review Comment:
I'm not sure this is right. maxNumberOfNonNullBuckets is a hard max, it
doesn't matter if things still reference old buckets and could backtrack to
them. We still need to allow those buckets to be garbage collected. If
something does backtrack into one of the garbage collected bucket then we throw
a BacktrackingException.
With this logic, we could get a PoU at the first byte that never gets
resolved and we would never garbage collect old buckets.
Note that this means we can only go so far in looking for EOD. Maybe we have
an off-by-one error somwehere, or maybe the math to figure out the target EOD
bucket needs to be fixed so we don't look for buckets too far in the future
that cause our current buckets to get garbage collected.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +95,149 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def optEffectiveLengthUnits(optLastNonEOPELU: Option[LengthUnits]):
Option[LengthUnits] = {
+ this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPELU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case LengthKind.Implicit | LengthKind.Delimited =>
+ None // invalid parent; SDE fires separately
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // Sequences are transparent — the ELU is inherited from the nearest
enclosing box.
+ case _: SequenceTermBase => optLastNonEOPELU
+ case _: ChoiceTermBase => None // implicit-length choice; SDE fires
separately
+ }
+ }
+
+ final lazy val childrenEndOfParent: Seq[ElementBase] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(lastNonEOPELU: Option[LengthUnits]):
Boolean = {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ lazy val optParentELU = term.optEffectiveLengthUnits(lastNonEOPELU)
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
Review Comment:
This can be lastNoneEPELU. We should use whatever we initialize
lastNoneEOPELU with, which is Characters for the root element.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/ParserTraits.scala:
##########
@@ -170,18 +171,82 @@ trait PrefixedLengthParserMixin {
* Some parsers do not calculate their own length, but instead expect another
parser
* to set the bit limit, and then they use that bit limit as the length.
* An example of this is prefix length parsers. This trait can be used by those
- * parsers to do determine the length based on the bitLimit and position.
+ * parsers to determine the length based on the bitLimit and position.
+ *
+ * For dfdl:lengthKind='endOfParent' parsers that need to scan to end-of-stream
+ * when no bit limit is set, mix in [[EndOfParentBitLengthMixin]] instead.
*/
trait BitLengthFromBitLimitMixin {
- def getBitLength(s: ParseOrUnparseState): Int = {
- val pState = s.asInstanceOf[PState]
- val len = getLengthInBits(pState)
- len.toInt
+ def getBitLength(s: ParseOrUnparseState): Int =
getBitLengthAsInt(s.asInstanceOf[PState])
+
+ /**
+ * getLengthInBits converted to Int with a parse error on overflow.
+ *
+ * The default maxCacheSizeInBytes (256 MiB) yields a maximum bit length of
+ * exactly Int.MaxValue + 1, so a bare .toInt without this guard can overflow
+ * by one bit on a full-cache EOP element.
+ */
+ def getBitLengthAsInt(pstate: PState): Int = {
+ val len = getLengthInBits(pstate)
+ if (pstate.processorStatus ne Success) return 0
+ if (len > Int.MaxValue) {
+ pstate.setFailed(
+ new ParseError(
+ One(pstate.schemaFileLocation),
+ One(pstate.currentLocation),
+ "Bit length %d exceeds maximum (%d) for this parser.",
+ len,
+ Int.MaxValue
+ )
+ )
+ 0
+ } else {
+ len.toInt
+ }
}
def getLengthInBits(pstate: PState): Long = {
- val len = pstate.bitLimit0b.get - pstate.bitPos0b
- len
+ if (pstate.bitLimit0b.isDefined) {
+ pstate.bitLimit0b.get - pstate.bitPos0b
+ } else {
+ Assert.invariantFailed("BitLimit not set for parser.")
+ }
+ }
+}
+
+/**
+ * Extends [[BitLengthFromBitLimitMixin]] with dfdl:lengthKind='endOfParent'
support.
+ * When no bit limit is set (i.e., the parser is at the root or outermost EOP
element),
+ * falls back to [[org.apache.daffodil.io.InputSource#optEndOfDataPosition]]
to locate
+ * the end of the data stream.
+ *
+ * Only mix this in for parsers that are instantiated specifically for EOP
elements.
+ * Parsers for prefixed or explicitly-bounded elements should use the plain
+ * [[BitLengthFromBitLimitMixin]]; mixing in this trait by mistake would
silently
+ * consume the entire remaining stream instead of failing at the missing bit
limit.
+ */
+trait EndOfParentBitLengthMixin extends BitLengthFromBitLimitMixin {
+
+ override def getLengthInBits(pstate: PState): Long = {
+ if (pstate.bitLimit0b.isDefined) {
+ pstate.bitLimit0b.get - pstate.bitPos0b
+ } else {
+ val dis = pstate.dataInputStream
+ dis.optEndOfDataPosition match {
+ case Some(endOfDataPosition) =>
+ (endOfDataPosition * 8) - dis.bitPos0b
+ case None =>
+ pstate.setFailed(
+ new ParseError(
+ One(pstate.schemaFileLocation),
+ One(pstate.currentLocation),
+ "Cannot determine end-of-data position for
dfdl:lengthKind='endOfParent': " +
+ "data stream exceeds the maximum cache size. Consider
increasing maxCacheSizeInBytes."
Review Comment:
I'm hesitant about this. For one `maxCacheSizeInBytes` is an implementation
defined limit of the BucketingInputSource, so this is kindof assuming the
failure is due to that. Additionally, there is now way for users to change this
via the public API. Users could use private APIs, but we dont' recommend that.
So it's a suggestion that pretty much no one can actually use.
We probably do want to fix the issue to at least allow API users to set the
cache size, but until then this diagnostic isn't really actionable for users.
Maybe we just say the data position and use knownBytesAvailable() to include
how many bytes were were able to scan from that position before giving up.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SpecifiedLengthParsers.scala:
##########
@@ -132,6 +149,85 @@ class SpecifiedLengthPatternParser(
}
}
+class SpecifiedLengthEndOfParentParser(
+ eParser: Parser,
+ erd: ElementRuntimeData
+) extends SpecifiedLengthParserBase(eParser, erd),
+ EndOfParentBitLengthMixin {
+
+ override protected def getBitLength(s: PState): MaybeULong = {
+ MaybeULong(getLengthInBits(s))
+ }
+
+ override def parse(pState: PState): Unit = {
+ val dis = pState.dataInputStream
+
+ if (erd.isComplexType) {
+ if (dis.bitLimit0b.isDefined) {
+ val (nBits: Long, dis: InputSourceDataInputStream, startingBitPos0b:
Long) =
+ checkLengthAndParseWithinBitLimits(
+ pState,
+ shouldCheckDefinedForLength = false
+ ) match {
+ case Some(result) => result
+ case None => return
+ }
+
+ // at this point the recursive parse of the children is finished
+ // so if we're still successful we need to advance the position
+ // to skip past any bits that the recursive child parse did not
+ // consume at the end. That is, the specified length can be an
+ // outer constraint, but the children may not use it all up, leaving
+ // a section at the end.
+ if (pState.processorStatus ne Success) return
+ val finalEndPos0b = startingBitPos0b + nBits
+
+ // we want to capture the length before we do any skipping
+ // value length of simple types is captured by the eParser
+ if (pState.infoset.isComplex)
+ captureValueLength(pState, ULong(startingBitPos0b),
ULong(dis.bitPos0b))
+
+ Assert.invariant(dis eq pState.dataInputStream)
+ val bitsToSkip = finalEndPos0b - dis.bitPos0b
+ // if this is < 0, then the parsing of children went past the limit,
which it isn't supposed to.
+ skipBits(pState, bitsToSkip, dis)
+ } else {
+ val startingBitPos0b = dis.bitPos0b
+
+ eParser.parse1(pState)
+
+ // at this point the recursive parse of the children is finished
+ // so if we're still successful we need to advance the position
+ // to skip past any bits that the recursive child parse did not
+ // consume at the end. That is, the specified length can be an
+ // outer constraint, but the children may not use it all up, leaving
+ // a section at the end.
+ if (pState.processorStatus ne Success) return
+
+ // we want to capture the length before we do any skipping
+ // value length of simple types is captured by the eParser
+ if (pState.infoset.isComplex) {
Review Comment:
If we have gotten here this must be comlpex so we can skip this check.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Term.scala:
##########
@@ -562,4 +560,11 @@ trait Term
}
}
+ final lazy val realElementChildren: Seq[ElementBase] = {
Review Comment:
Should the name of this reference EOP somehow? The fact that this does not
descend into choices is specifically because this is used for EOP and is
different from how realChildren works.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -629,6 +630,9 @@ trait ElementBaseGrammarMixin
)
}
+ private lazy val unsupportedLengthKindEOPMessage =
+ "lengthKind='endOfParent' only supported for packed binary formats."
+
Review Comment:
While I agree it's nice to avoid duplication, our convention is kindof to
duplicate error messages all over the place. And this is only used in three
places, so it's not a ton of duplciatino this is avoiding.
What we really need is some sort of error message lookup table system, where
all error messages get an identifier. But until that point, I would suggest we
just duplicate this message in the few places it's used.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -252,6 +253,60 @@ trait ElementBaseGrammarMixin
}
final lazy val prefixedLengthBody = prefixedLengthElementDecl.parsedValue
+ def checkEndOfParentRestrictionsOnCurrentElement(optParentELU:
Option[LengthUnits]): Unit = {
+ Assert.invariant(optParentELU.isDefined)
+ val parentELU = optParentELU.get
+ val currentElement: ElementBase = this
+ Assert.invariant(currentElement.lengthKind == LengthKind.EndOfParent)
+ schemaDefinitionWhen(
+ currentElement.hasTerminator,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but specifies
a dfdl:terminator."
+ )
+ schemaDefinitionWhen(
+ currentElement.trailingSkip != 0,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but specifies
a non-zero dfdl:trailingSkip."
+ )
+ schemaDefinitionWhen(
+ currentElement.maxOccurs > 1,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but specifies
a maxOccurs greater than 1."
+ )
+ schemaDefinitionWhen(
+ currentElement.impliedRepresentation == Representation.Text
+ && (!currentElement.isKnownEncoding ||
!currentElement.knownEncodingIsFixedWidth ||
currentElement.knownEncodingWidthInBits != 8)
+ && (parentELU != LengthUnits.Characters),
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but the
element has text representation, and does not have a single-byte character set
encoding, and the effective length units of the parent is not 'characters'."
+ )
+ if (currentElement.isSimpleType) {
+ val meetsSimpleTypeRestrictions = (currentElement.primType eq
PrimType.String)
+ || (currentElement.representation == Representation.Text)
+ || (currentElement.primType eq PrimType.HexBinary)
+ || (currentElement.representation == Representation.Binary
+ && ((
+ currentElement.binaryNumberRep,
Review Comment:
You need to look at the type to know if you you should look at
binaryNubmerRep or binaryCalendarRep. For example, if the type is not
date/dateTime/time, then you dont' care about the value of binaryCalendarRep
since it will be ignored.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +95,149 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def optEffectiveLengthUnits(optLastNonEOPELU: Option[LengthUnits]):
Option[LengthUnits] = {
+ this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPELU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case LengthKind.Implicit | LengthKind.Delimited =>
+ None // invalid parent; SDE fires separately
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // Sequences are transparent — the ELU is inherited from the nearest
enclosing box.
+ case _: SequenceTermBase => optLastNonEOPELU
+ case _: ChoiceTermBase => None // implicit-length choice; SDE fires
separately
+ }
+ }
+
+ final lazy val childrenEndOfParent: Seq[ElementBase] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(lastNonEOPELU: Option[LengthUnits]):
Boolean = {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ lazy val optParentELU = term.optEffectiveLengthUnits(lastNonEOPELU)
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
Review Comment:
I'm still not convinced we need this separate case. With suggested changes
below, the logic holds for both root and non-root elements. The only addition
if a condition to call checkEndOfParentRestrictionsOnCurrentElement on the
rootELem.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +95,149 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def optEffectiveLengthUnits(optLastNonEOPELU: Option[LengthUnits]):
Option[LengthUnits] = {
+ this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPELU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case LengthKind.Implicit | LengthKind.Delimited =>
+ None // invalid parent; SDE fires separately
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // Sequences are transparent — the ELU is inherited from the nearest
enclosing box.
+ case _: SequenceTermBase => optLastNonEOPELU
+ case _: ChoiceTermBase => None // implicit-length choice; SDE fires
separately
+ }
+ }
+
+ final lazy val childrenEndOfParent: Seq[ElementBase] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(lastNonEOPELU: Option[LengthUnits]):
Boolean = {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ lazy val optParentELU = term.optEffectiveLengthUnits(lastNonEOPELU)
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach { child =>
+ child.checkEndOfParentRestrictionsOnCurrentElement(lastNonEOPELU)
Review Comment:
This can pass in optParentELU. For a root element with EOP optParentELU ends
up being lastNonEOPELU, so it'sfunctionally the same, but it makes it so all
calls to `checkEndOfParentRestrictionsOnCurrentElement` have the same
optParentELU parameter.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +95,149 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def optEffectiveLengthUnits(optLastNonEOPELU: Option[LengthUnits]):
Option[LengthUnits] = {
+ this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPELU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case LengthKind.Implicit | LengthKind.Delimited =>
+ None // invalid parent; SDE fires separately
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // Sequences are transparent — the ELU is inherited from the nearest
enclosing box.
+ case _: SequenceTermBase => optLastNonEOPELU
+ case _: ChoiceTermBase => None // implicit-length choice; SDE fires
separately
+ }
+ }
+
+ final lazy val childrenEndOfParent: Seq[ElementBase] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(lastNonEOPELU: Option[LengthUnits]):
Boolean = {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ lazy val optParentELU = term.optEffectiveLengthUnits(lastNonEOPELU)
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach { child =>
+ child.checkEndOfParentRestrictionsOnCurrentElement(lastNonEOPELU)
+ }
+ }
+ case e: ElementBase if eopChildren.nonEmpty => {
Review Comment:
We an remove the `eopChildren.nonEmpty` checks in all of these cases. If
it's empty the eopChildren.foreach loops will just be no-ops.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/ParserTraits.scala:
##########
@@ -170,18 +171,82 @@ trait PrefixedLengthParserMixin {
* Some parsers do not calculate their own length, but instead expect another
parser
* to set the bit limit, and then they use that bit limit as the length.
* An example of this is prefix length parsers. This trait can be used by those
- * parsers to do determine the length based on the bitLimit and position.
+ * parsers to determine the length based on the bitLimit and position.
+ *
+ * For dfdl:lengthKind='endOfParent' parsers that need to scan to end-of-stream
+ * when no bit limit is set, mix in [[EndOfParentBitLengthMixin]] instead.
*/
trait BitLengthFromBitLimitMixin {
- def getBitLength(s: ParseOrUnparseState): Int = {
- val pState = s.asInstanceOf[PState]
- val len = getLengthInBits(pState)
- len.toInt
+ def getBitLength(s: ParseOrUnparseState): Int =
getBitLengthAsInt(s.asInstanceOf[PState])
+
+ /**
+ * getLengthInBits converted to Int with a parse error on overflow.
+ *
+ * The default maxCacheSizeInBytes (256 MiB) yields a maximum bit length of
Review Comment:
I don't think maxCacheSizeInBytes is relevant here. That is an
implementation detail of the bucketing input source and can be configured. Even
with the ByteArrayInputSource, which desn't have a concept of a max cache size,
we could theoretically overflow if we had a length in bits over 4GB.
That said, we might have a bug if we are using Int's, we should probably be
using Longs to avoid this overflow. Weuse long for getLengthInBits, so I'm not
sure whey we would ever want to convert that to a Int. Might be worth opening a
ticket.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/ParserTraits.scala:
##########
@@ -170,18 +171,82 @@ trait PrefixedLengthParserMixin {
* Some parsers do not calculate their own length, but instead expect another
parser
* to set the bit limit, and then they use that bit limit as the length.
* An example of this is prefix length parsers. This trait can be used by those
- * parsers to do determine the length based on the bitLimit and position.
+ * parsers to determine the length based on the bitLimit and position.
+ *
+ * For dfdl:lengthKind='endOfParent' parsers that need to scan to end-of-stream
+ * when no bit limit is set, mix in [[EndOfParentBitLengthMixin]] instead.
*/
trait BitLengthFromBitLimitMixin {
- def getBitLength(s: ParseOrUnparseState): Int = {
- val pState = s.asInstanceOf[PState]
- val len = getLengthInBits(pState)
- len.toInt
+ def getBitLength(s: ParseOrUnparseState): Int =
getBitLengthAsInt(s.asInstanceOf[PState])
+
+ /**
+ * getLengthInBits converted to Int with a parse error on overflow.
+ *
+ * The default maxCacheSizeInBytes (256 MiB) yields a maximum bit length of
+ * exactly Int.MaxValue + 1, so a bare .toInt without this guard can overflow
+ * by one bit on a full-cache EOP element.
+ */
+ def getBitLengthAsInt(pstate: PState): Int = {
+ val len = getLengthInBits(pstate)
+ if (pstate.processorStatus ne Success) return 0
+ if (len > Int.MaxValue) {
+ pstate.setFailed(
+ new ParseError(
Review Comment:
I think this might want to be an SDE. We shoudl double check what we do in
other places where things exceeed implementation limits, but I don't think we
usually allow backtracking.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/ParserTraits.scala:
##########
@@ -170,18 +171,82 @@ trait PrefixedLengthParserMixin {
* Some parsers do not calculate their own length, but instead expect another
parser
* to set the bit limit, and then they use that bit limit as the length.
* An example of this is prefix length parsers. This trait can be used by those
- * parsers to do determine the length based on the bitLimit and position.
+ * parsers to determine the length based on the bitLimit and position.
+ *
+ * For dfdl:lengthKind='endOfParent' parsers that need to scan to end-of-stream
+ * when no bit limit is set, mix in [[EndOfParentBitLengthMixin]] instead.
*/
trait BitLengthFromBitLimitMixin {
- def getBitLength(s: ParseOrUnparseState): Int = {
- val pState = s.asInstanceOf[PState]
- val len = getLengthInBits(pState)
- len.toInt
+ def getBitLength(s: ParseOrUnparseState): Int =
getBitLengthAsInt(s.asInstanceOf[PState])
+
+ /**
+ * getLengthInBits converted to Int with a parse error on overflow.
+ *
+ * The default maxCacheSizeInBytes (256 MiB) yields a maximum bit length of
+ * exactly Int.MaxValue + 1, so a bare .toInt without this guard can overflow
+ * by one bit on a full-cache EOP element.
+ */
+ def getBitLengthAsInt(pstate: PState): Int = {
+ val len = getLengthInBits(pstate)
+ if (pstate.processorStatus ne Success) return 0
+ if (len > Int.MaxValue) {
+ pstate.setFailed(
+ new ParseError(
+ One(pstate.schemaFileLocation),
+ One(pstate.currentLocation),
+ "Bit length %d exceeds maximum (%d) for this parser.",
Review Comment:
I don't think we usualy say "this parser". That isn't really meaningful to
users. I think it's sufficient to just say the limit exceeededthe maximum
value. We probably have standard language we could copy, we have other limits
through the code.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/ParserTraits.scala:
##########
@@ -170,18 +171,82 @@ trait PrefixedLengthParserMixin {
* Some parsers do not calculate their own length, but instead expect another
parser
* to set the bit limit, and then they use that bit limit as the length.
* An example of this is prefix length parsers. This trait can be used by those
- * parsers to do determine the length based on the bitLimit and position.
+ * parsers to determine the length based on the bitLimit and position.
+ *
+ * For dfdl:lengthKind='endOfParent' parsers that need to scan to end-of-stream
+ * when no bit limit is set, mix in [[EndOfParentBitLengthMixin]] instead.
*/
trait BitLengthFromBitLimitMixin {
- def getBitLength(s: ParseOrUnparseState): Int = {
- val pState = s.asInstanceOf[PState]
- val len = getLengthInBits(pState)
- len.toInt
+ def getBitLength(s: ParseOrUnparseState): Int =
getBitLengthAsInt(s.asInstanceOf[PState])
+
+ /**
+ * getLengthInBits converted to Int with a parse error on overflow.
+ *
+ * The default maxCacheSizeInBytes (256 MiB) yields a maximum bit length of
+ * exactly Int.MaxValue + 1, so a bare .toInt without this guard can overflow
+ * by one bit on a full-cache EOP element.
+ */
+ def getBitLengthAsInt(pstate: PState): Int = {
+ val len = getLengthInBits(pstate)
+ if (pstate.processorStatus ne Success) return 0
+ if (len > Int.MaxValue) {
+ pstate.setFailed(
+ new ParseError(
+ One(pstate.schemaFileLocation),
+ One(pstate.currentLocation),
+ "Bit length %d exceeds maximum (%d) for this parser.",
+ len,
+ Int.MaxValue
+ )
+ )
+ 0
+ } else {
+ len.toInt
+ }
}
def getLengthInBits(pstate: PState): Long = {
- val len = pstate.bitLimit0b.get - pstate.bitPos0b
- len
+ if (pstate.bitLimit0b.isDefined) {
+ pstate.bitLimit0b.get - pstate.bitPos0b
+ } else {
+ Assert.invariantFailed("BitLimit not set for parser.")
+ }
+ }
+}
+
+/**
+ * Extends [[BitLengthFromBitLimitMixin]] with dfdl:lengthKind='endOfParent'
support.
+ * When no bit limit is set (i.e., the parser is at the root or outermost EOP
element),
+ * falls back to [[org.apache.daffodil.io.InputSource#optEndOfDataPosition]]
to locate
+ * the end of the data stream.
+ *
+ * Only mix this in for parsers that are instantiated specifically for EOP
elements.
+ * Parsers for prefixed or explicitly-bounded elements should use the plain
+ * [[BitLengthFromBitLimitMixin]]; mixing in this trait by mistake would
silently
+ * consume the entire remaining stream instead of failing at the missing bit
limit.
+ */
+trait EndOfParentBitLengthMixin extends BitLengthFromBitLimitMixin {
+
+ override def getLengthInBits(pstate: PState): Long = {
+ if (pstate.bitLimit0b.isDefined) {
+ pstate.bitLimit0b.get - pstate.bitPos0b
+ } else {
+ val dis = pstate.dataInputStream
+ dis.optEndOfDataPosition match {
+ case Some(endOfDataPosition) =>
+ (endOfDataPosition * 8) - dis.bitPos0b
+ case None =>
+ pstate.setFailed(
+ new ParseError(
Review Comment:
I think this wants to be a SDE. I think hitteing implementation defines
limits is not backtrackable.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SpecifiedLengthParsers.scala:
##########
@@ -87,12 +76,39 @@ sealed abstract class SpecifiedLengthParserBase(eParser:
Parser, erd: RuntimeDat
// we want to capture the length before we do any skipping
// value length of simple types is captured by the eParser if needed
- // the SpecifiedLengthParserBase is extended by
SpecifiedLengthChoiceParser which should not have its valueLength captured here
+ // the SpecifiedLengthParserBase is extended by SpecifiedLengthChoiceParser
+ // which should not have its valueLength captured here
if (pState.infoset.isComplex && !erd.isInstanceOf[ChoiceRuntimeData])
captureValueLength(pState, ULong(startingBitPos0b), ULong(dis.bitPos0b))
Assert.invariant(dis eq pState.dataInputStream)
val bitsToSkip = finalEndPos0b - dis.bitPos0b
+ skipBits(pState, bitsToSkip, dis)
+ }
+
+ protected def checkLengthAndParseWithinBitLimits(
+ pState: PState,
+ shouldCheckDefinedForLength: Boolean
+ ): Option[(Long, InputSourceDataInputStream, Long)] = {
+ val maybeNBits = getBitLength(pState)
+
+ if (pState.processorStatus ne Success) return None
+ val nBits = maybeNBits.get
+ val dis = pState.dataInputStream
+
+ if (shouldCheckDefinedForLength && !dis.isDefinedForLength(nBits)) {
+ PENotEnoughBits(pState, nBits, dis)
+ return None
+ }
+
+ val startingBitPos0b = dis.bitPos0b
+ dis.withBitLengthLimit(nBits) {
+ eParser.parse1(pState)
+ }
+ Some((nBits, dis, startingBitPos0b))
Review Comment:
Do we need to return dis and startingBitPosition? Can those be captured by
the caller before calling this function? Then we only need to return nBits so
we don't need to return a tuple.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SpecifiedLengthParsers.scala:
##########
@@ -52,29 +52,18 @@ sealed abstract class SpecifiedLengthParserBase(eParser:
Parser, erd: RuntimeDat
*/
protected def getBitLength(s: PState): MaybeULong
- final def parse(pState: PState): Unit = {
-
- val maybeNBits = getBitLength(pState)
-
- if (pState.processorStatus._ne_(Success)) return
- val nBits = maybeNBits.get
- val dis = pState.dataInputStream
-
- val shouldCheckDefinedForLength = erd match {
+ def parse(pState: PState): Unit = {
+ lazy val shouldCheckDefinedForLength = erd match {
Review Comment:
This doesn't need to be lazy, we will always evaluate this.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SpecifiedLengthParsers.scala:
##########
@@ -132,6 +149,85 @@ class SpecifiedLengthPatternParser(
}
}
+class SpecifiedLengthEndOfParentParser(
+ eParser: Parser,
+ erd: ElementRuntimeData
+) extends SpecifiedLengthParserBase(eParser, erd),
+ EndOfParentBitLengthMixin {
+
+ override protected def getBitLength(s: PState): MaybeULong = {
+ MaybeULong(getLengthInBits(s))
+ }
+
+ override def parse(pState: PState): Unit = {
+ val dis = pState.dataInputStream
+
+ if (erd.isComplexType) {
+ if (dis.bitLimit0b.isDefined) {
+ val (nBits: Long, dis: InputSourceDataInputStream, startingBitPos0b:
Long) =
+ checkLengthAndParseWithinBitLimits(
+ pState,
+ shouldCheckDefinedForLength = false
+ ) match {
+ case Some(result) => result
+ case None => return
+ }
+
+ // at this point the recursive parse of the children is finished
+ // so if we're still successful we need to advance the position
+ // to skip past any bits that the recursive child parse did not
+ // consume at the end. That is, the specified length can be an
+ // outer constraint, but the children may not use it all up, leaving
+ // a section at the end.
+ if (pState.processorStatus ne Success) return
+ val finalEndPos0b = startingBitPos0b + nBits
+
+ // we want to capture the length before we do any skipping
+ // value length of simple types is captured by the eParser
+ if (pState.infoset.isComplex)
+ captureValueLength(pState, ULong(startingBitPos0b),
ULong(dis.bitPos0b))
+
+ Assert.invariant(dis eq pState.dataInputStream)
+ val bitsToSkip = finalEndPos0b - dis.bitPos0b
+ // if this is < 0, then the parsing of children went past the limit,
which it isn't supposed to.
+ skipBits(pState, bitsToSkip, dis)
+ } else {
+ val startingBitPos0b = dis.bitPos0b
+
+ eParser.parse1(pState)
+
+ // at this point the recursive parse of the children is finished
+ // so if we're still successful we need to advance the position
+ // to skip past any bits that the recursive child parse did not
+ // consume at the end. That is, the specified length can be an
+ // outer constraint, but the children may not use it all up, leaving
+ // a section at the end.
+ if (pState.processorStatus ne Success) return
+
+ // we want to capture the length before we do any skipping
+ // value length of simple types is captured by the eParser
+ if (pState.infoset.isComplex) {
+ captureValueLength(pState, ULong(startingBitPos0b),
ULong(dis.bitPos0b))
+ }
+
+ val maybeNBits = getBitLength(pState)
+
+ if (pState.processorStatus ne Success) return
+ val nBits = maybeNBits.get
+
+ if (!dis.isDefinedForLength(nBits)) {
Review Comment:
We dont' need this check. If we got a result it means we hit EOD and so are
guarnateed to know we are defined for that length.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SpecifiedLengthParsers.scala:
##########
@@ -132,6 +149,85 @@ class SpecifiedLengthPatternParser(
}
}
+class SpecifiedLengthEndOfParentParser(
+ eParser: Parser,
+ erd: ElementRuntimeData
+) extends SpecifiedLengthParserBase(eParser, erd),
+ EndOfParentBitLengthMixin {
+
+ override protected def getBitLength(s: PState): MaybeULong = {
+ MaybeULong(getLengthInBits(s))
+ }
+
+ override def parse(pState: PState): Unit = {
+ val dis = pState.dataInputStream
+
+ if (erd.isComplexType) {
+ if (dis.bitLimit0b.isDefined) {
+ val (nBits: Long, dis: InputSourceDataInputStream, startingBitPos0b:
Long) =
+ checkLengthAndParseWithinBitLimits(
+ pState,
+ shouldCheckDefinedForLength = false
+ ) match {
+ case Some(result) => result
+ case None => return
+ }
+
+ // at this point the recursive parse of the children is finished
+ // so if we're still successful we need to advance the position
+ // to skip past any bits that the recursive child parse did not
+ // consume at the end. That is, the specified length can be an
+ // outer constraint, but the children may not use it all up, leaving
+ // a section at the end.
+ if (pState.processorStatus ne Success) return
+ val finalEndPos0b = startingBitPos0b + nBits
+
+ // we want to capture the length before we do any skipping
+ // value length of simple types is captured by the eParser
+ if (pState.infoset.isComplex)
+ captureValueLength(pState, ULong(startingBitPos0b),
ULong(dis.bitPos0b))
+
+ Assert.invariant(dis eq pState.dataInputStream)
+ val bitsToSkip = finalEndPos0b - dis.bitPos0b
+ // if this is < 0, then the parsing of children went past the limit,
which it isn't supposed to.
+ skipBits(pState, bitsToSkip, dis)
+ } else {
+ val startingBitPos0b = dis.bitPos0b
Review Comment:
I think we need to comment explain why we have this separate logic for the
complex + no-bit limit case. Something that says that getLength in that case
will read to EOD, which we don't want to do for complex types because EOD could
be very far away. Instead we just let the children parse as much as they want,
and after that we figure out how much is left over to EOD and skip it
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SpecifiedLengthParsers.scala:
##########
@@ -132,6 +149,85 @@ class SpecifiedLengthPatternParser(
}
}
+class SpecifiedLengthEndOfParentParser(
+ eParser: Parser,
+ erd: ElementRuntimeData
+) extends SpecifiedLengthParserBase(eParser, erd),
+ EndOfParentBitLengthMixin {
+
+ override protected def getBitLength(s: PState): MaybeULong = {
+ MaybeULong(getLengthInBits(s))
+ }
+
+ override def parse(pState: PState): Unit = {
+ val dis = pState.dataInputStream
+
+ if (erd.isComplexType) {
+ if (dis.bitLimit0b.isDefined) {
+ val (nBits: Long, dis: InputSourceDataInputStream, startingBitPos0b:
Long) =
Review Comment:
I think we can call super.parse() in this case too. If bit limit is defined
then getLength won't try to scan for EOD, it'll just return bit limit. I we did
that, I think that would simplify this quite a bit and would make it so you
wouldn't need a good chunk of the above changes. For example, I think the
`checkLengthAndParseWithinBitLimits` function is no longer needed since it's
only called by the super parser function.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SpecifiedLengthParsers.scala:
##########
@@ -132,6 +149,85 @@ class SpecifiedLengthPatternParser(
}
}
+class SpecifiedLengthEndOfParentParser(
+ eParser: Parser,
+ erd: ElementRuntimeData
+) extends SpecifiedLengthParserBase(eParser, erd),
+ EndOfParentBitLengthMixin {
+
+ override protected def getBitLength(s: PState): MaybeULong = {
+ MaybeULong(getLengthInBits(s))
+ }
+
+ override def parse(pState: PState): Unit = {
+ val dis = pState.dataInputStream
+
+ if (erd.isComplexType) {
+ if (dis.bitLimit0b.isDefined) {
+ val (nBits: Long, dis: InputSourceDataInputStream, startingBitPos0b:
Long) =
+ checkLengthAndParseWithinBitLimits(
+ pState,
+ shouldCheckDefinedForLength = false
+ ) match {
+ case Some(result) => result
+ case None => return
+ }
+
+ // at this point the recursive parse of the children is finished
+ // so if we're still successful we need to advance the position
+ // to skip past any bits that the recursive child parse did not
+ // consume at the end. That is, the specified length can be an
+ // outer constraint, but the children may not use it all up, leaving
+ // a section at the end.
+ if (pState.processorStatus ne Success) return
+ val finalEndPos0b = startingBitPos0b + nBits
+
+ // we want to capture the length before we do any skipping
+ // value length of simple types is captured by the eParser
+ if (pState.infoset.isComplex)
+ captureValueLength(pState, ULong(startingBitPos0b),
ULong(dis.bitPos0b))
+
+ Assert.invariant(dis eq pState.dataInputStream)
+ val bitsToSkip = finalEndPos0b - dis.bitPos0b
+ // if this is < 0, then the parsing of children went past the limit,
which it isn't supposed to.
+ skipBits(pState, bitsToSkip, dis)
+ } else {
+ val startingBitPos0b = dis.bitPos0b
+
+ eParser.parse1(pState)
+
+ // at this point the recursive parse of the children is finished
+ // so if we're still successful we need to advance the position
+ // to skip past any bits that the recursive child parse did not
+ // consume at the end. That is, the specified length can be an
+ // outer constraint, but the children may not use it all up, leaving
+ // a section at the end.
+ if (pState.processorStatus ne Success) return
+
+ // we want to capture the length before we do any skipping
+ // value length of simple types is captured by the eParser
+ if (pState.infoset.isComplex) {
+ captureValueLength(pState, ULong(startingBitPos0b),
ULong(dis.bitPos0b))
+ }
+
+ val maybeNBits = getBitLength(pState)
Review Comment:
myabe add a comment here that says this will find remaining bits to EOD. It
might also be worth noting that because getBitLength is called after the child
parse this value does not indicate the length of the complex type like it
normall does, but instead indicates the number of bits until EOD.
--
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]
