stevedlawrence commented on code in PR #1652:
URL: https://github.com/apache/daffodil/pull/1652#discussion_r3357457134
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +96,181 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def myEffectiveLengthUnits(optLastNonEOPLU: Option[LengthUnits]):
Option[LengthUnits] = {
+ val elu = this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPLU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case _ => None
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // the spec doesn't actually account for this case, but logically it
makes
+ // sense that ELU of a sequence is the ELU of its parent that
technically is the
+ // last non-EndOfParent ELU.
+ case s: SequenceTermBase => optLastNonEOPLU
+ case _ => None
+ }
+ elu
+ }
+
+ final lazy val childrenEndOfParent: Seq[Term] =
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(optLastNonEOPLU: Option[LengthUnits]): Unit
= {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach {
+ case e: ElementBase => {
+ rootElem.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optLastNonEOPLU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case parent: ElementBase if eopChildren.nonEmpty => {
Review Comment:
I agree we need the two cases, it just feels like the logic in the two cases
are very similar except for the addition of a check of the root element. I
guess I was thinking something along these lines:
```scala
case elem: ElementBase => {
elem match {
case root: Root if root.lengthKind == LengthKind.EndOfParent =>
root.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
case _ => // no-op
}
if (eopChildren.nonEmpty) {
// existing ElementBase checks
...
eopChildren.foreach { ... }
}
}
```
So there's one case that handles all elements, which is the same regardless
of root or not, except for the one additional check for EOP roots.
That way we avoid duplicating the very similar eopChildren.foreach logic in
the two cases.
--
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]
