stevedlawrence commented on code in PR #1604:
URL: https://github.com/apache/daffodil/pull/1604#discussion_r2677494191
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -228,29 +245,162 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
}
}.value
+ private lazy val separatorPrefixMTAApprox =
Review Comment:
Is it worth calling this `separatorPrefixInfixMTAApprox`, just to make it
clear this is represents both of kinds of separators?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -228,29 +245,162 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
}
}.value
+ private lazy val separatorPrefixMTAApprox =
+ this.optLexicalParent match {
+ case Some(s: SequenceTermBase) if s.hasSeparator =>
+ import SeparatorPosition.*
+ s.separatorPosition match {
+ case Prefix | Infix =>
AlignmentMultipleOf(s.knownEncodingAlignmentInBits)
+ case Postfix => AlignmentMultipleOf(1)
+ }
+ case _ => AlignmentMultipleOf(1)
+ }
+
+ private lazy val separatorPostfixMTAApprox =
+ this.optLexicalParent match {
+ case Some(s: SequenceTermBase) if s.hasSeparator =>
+ import SeparatorPosition.*
+ s.separatorPosition match {
+ case Prefix | Infix => AlignmentMultipleOf(1)
+ case Postfix => AlignmentMultipleOf(s.knownEncodingAlignmentInBits)
+ }
+ case _ => AlignmentMultipleOf(1)
+ }
+
+ private lazy val separatorLengthApprox = this.optLexicalParent match {
+ case Some(s: SequenceTermBase) if s.hasSeparator =>
+ getEncodingLengthApprox(s)
+ case _ => LengthExact(0)
+ }
+
+ private lazy val initiatorMTAApprox =
+ if (this.hasInitiator) {
+ AlignmentMultipleOf(this.knownEncodingAlignmentInBits)
+ } else {
+ AlignmentMultipleOf(1)
+ }
+
+ private lazy val initiatorLengthApprox = if (this.hasInitiator) {
+ getEncodingLengthApprox(this)
+ } else {
+ LengthExact(0)
+ }
+
+ private lazy val terminatorMTAApprox =
+ if (this.hasTerminator) {
+ AlignmentMultipleOf(this.knownEncodingAlignmentInBits)
+ } else {
+ AlignmentMultipleOf(1)
+ }
+
+ private lazy val terminatorLengthApprox = if (this.hasTerminator) {
+ getEncodingLengthApprox(this)
+ } else {
+ LengthExact(0)
+ }
+
+ private def getEncodingLengthApprox(t: Term) = {
+ if (t.isKnownEncoding) {
+ val dfdlCharset = t.charsetEv.optConstant.get
+ val fixedWidth =
+ if (dfdlCharset.maybeFixedWidth.isDefined)
dfdlCharset.maybeFixedWidth.get else 8
+ LengthMultipleOf(fixedWidth)
+ } else {
+ // runtime encoding, which must be a byte-length encoding
+ LengthMultipleOf(8)
+ }
+ }
+
+ /**
+ * prior alignment with leading skip includes the prefix/infix separator MTA
and length,
+ * any leading skips
+ */
private lazy val priorAlignmentWithLeadingSkipApprox: AlignmentMultipleOf = {
- priorAlignmentApprox + leadingSkipApprox
+ val priorAlignmentWithSeparatorApprox = priorAlignmentApprox
+ + separatorPrefixMTAApprox
+ + separatorLengthApprox
+ val leadingAlignmentApprox = (priorAlignmentWithSeparatorApprox
+ + leadingSkipApprox)
+ leadingAlignmentApprox
}
- protected lazy val contentStartAlignment: AlignmentMultipleOf = {
- if (priorAlignmentWithLeadingSkipApprox % alignmentApprox == 0) {
- // alignment won't be needed, continue using prior alignment as start
alignment
- priorAlignmentWithLeadingSkipApprox
+ /**
+ * The length of the prefix length quasi element.
+ *
+ * This is only relevant for quasi elements, or prefixed terms.
+ */
+ protected lazy val prefixLengthElementLength: LengthApprox = {
+ this match {
+ case eb: ElementBase => {
+ if (eb.lengthKind == Prefixed) {
+ val prefixTypeElem = eb.prefixedLengthElementDecl
+ getElementLength(prefixTypeElem)
Review Comment:
Is it possible to use
`eb.prefixedLenghtElementDecl.elementSpecifiedLengthApprox` to get the length
of the prefix element instead of the new getElementLength?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -228,29 +245,162 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
}
}.value
+ private lazy val separatorPrefixMTAApprox =
+ this.optLexicalParent match {
+ case Some(s: SequenceTermBase) if s.hasSeparator =>
+ import SeparatorPosition.*
+ s.separatorPosition match {
+ case Prefix | Infix =>
AlignmentMultipleOf(s.knownEncodingAlignmentInBits)
+ case Postfix => AlignmentMultipleOf(1)
+ }
+ case _ => AlignmentMultipleOf(1)
+ }
+
+ private lazy val separatorPostfixMTAApprox =
+ this.optLexicalParent match {
+ case Some(s: SequenceTermBase) if s.hasSeparator =>
+ import SeparatorPosition.*
+ s.separatorPosition match {
+ case Prefix | Infix => AlignmentMultipleOf(1)
+ case Postfix => AlignmentMultipleOf(s.knownEncodingAlignmentInBits)
+ }
+ case _ => AlignmentMultipleOf(1)
+ }
+
+ private lazy val separatorLengthApprox = this.optLexicalParent match {
+ case Some(s: SequenceTermBase) if s.hasSeparator =>
+ getEncodingLengthApprox(s)
+ case _ => LengthExact(0)
+ }
+
+ private lazy val initiatorMTAApprox =
+ if (this.hasInitiator) {
+ AlignmentMultipleOf(this.knownEncodingAlignmentInBits)
+ } else {
+ AlignmentMultipleOf(1)
+ }
+
+ private lazy val initiatorLengthApprox = if (this.hasInitiator) {
+ getEncodingLengthApprox(this)
+ } else {
+ LengthExact(0)
+ }
+
+ private lazy val terminatorMTAApprox =
+ if (this.hasTerminator) {
+ AlignmentMultipleOf(this.knownEncodingAlignmentInBits)
+ } else {
+ AlignmentMultipleOf(1)
+ }
+
+ private lazy val terminatorLengthApprox = if (this.hasTerminator) {
+ getEncodingLengthApprox(this)
+ } else {
+ LengthExact(0)
+ }
+
+ private def getEncodingLengthApprox(t: Term) = {
Review Comment:
Could we make this a private lazy val so we don't have to recalculate it?
For example, uses would look like `term.encodingLengthApprox` instead of
`getEncodingLengthApprox(someTerm)`?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -260,37 +410,59 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
//
val (lastChildren, couldBeLast) = mg.potentialLastChildren
val lastApproxesConsideringChildren: Seq[AlignmentMultipleOf] =
lastChildren.map { lc =>
- //
- // for each possible last child, add its ending alignment
- // to our trailing skip to get where it would leave off were
- // it the actual last child.
- //
+ // for each possible last child, gather its endingAlignmentApprox
val lceaa = lc.endingAlignmentApprox
Review Comment:
Does this want to be `endingAlignmentWithRightFramingApprox`? This case is
trying to figure out the ending alignment of the last child(ren) of the model
group, which is where the content of this model group ends. If the last
child(ren) contains a postfix separator, then I think that wants to be included
as part of the content of this model group. And so this wants to use the
.`..WithRightFraming` variable?
Or, maybe we just move the `separatorPostfixMTAApprox` and
`separatorLengthApprox` additions into `endingAlignmentAppprox` and remove the
`...WithRightFraming` variable? Since with the above change, nothing would
actually access `...WithRightFraming` anymore?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -260,37 +410,59 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
//
val (lastChildren, couldBeLast) = mg.potentialLastChildren
val lastApproxesConsideringChildren: Seq[AlignmentMultipleOf] =
lastChildren.map { lc =>
- //
- // for each possible last child, add its ending alignment
- // to our trailing skip to get where it would leave off were
- // it the actual last child.
- //
+ // for each possible last child, gather its endingAlignmentApprox
val lceaa = lc.endingAlignmentApprox
- val res = lceaa + trailingSkipApprox
- res
+ lceaa
}
val optApproxIfNoChildren =
//
// gather possibilities for this item itself
//
- if (couldBeLast)
+ if (couldBeLast) {
//
// if this model group could be last, then consider
// if none of its content was present.
- // We'd just have the contentStart, nothing, and the trailing Skip.
+ // We'd just have the contentStart
//
- Some(contentStartAlignment + trailingSkipApprox)
- else
+ val res = Some(contentStartAlignment)
+ res
+ } else
// can't be last, no possibilities to gather.
None
val lastApproxes = lastApproxesConsideringChildren ++
optApproxIfNoChildren
+ // take all the gathered possibilities to get where it would leave off
were
+ // each the actual last child.
Assert.invariant(lastApproxes.nonEmpty)
- val res = lastApproxes.reduce { _ * _ }
+ val res = lastApproxes.reduce {
+ _ * _
Review Comment:
Can we put this all on one line? Looks confusing having a line with just
symbol characters.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -312,34 +484,41 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
LengthMultipleOf(8)
}
} else
- eb.lengthUnits match {
- case LengthUnits.Bits => LengthMultipleOf(1)
- case LengthUnits.Bytes => LengthMultipleOf(8)
- case LengthUnits.Characters => encodingLengthApprox
- }
+ getApproxEncodingLengthForLengthUnit(eb)
}
case LengthKind.Delimited => encodingLengthApprox
case LengthKind.Pattern => encodingLengthApprox
case LengthKind.EndOfParent => LengthMultipleOf(1) // NYI
- case LengthKind.Prefixed => LengthMultipleOf(1) // NYI
+ case LengthKind.Prefixed => {
+ val prefixElem = eb.prefixedLengthElementDecl
+ if (prefixElem.lengthKind == Explicit || prefixElem.lengthKind ==
Implicit) {
+ LengthExact(
+ prefixElem.elementLengthInBitsEv.optConstant.get.get
Review Comment:
I think this might be incorrect? This is saying that if an element is
lengthKind="prefix" then the elements length is length of the prefixed length
element. But that's not quite right. The length of this element is the length
of the *value* of the prefix element, which can't be known until runtime. So we
can never know the exact length of a prefix length element.
I might suggest we just remove the new prefix length logic (maybe add
comments where logic is missing) and leave it as NYI. I think prefix length
elements are broken in a number of places so it's going to be hard to test if
this is even working correctly. Suggest we fix this when we fix prefixed
lengths.
--
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]
