jadams-tresys commented on code in PR #1604:
URL: https://github.com/apache/daffodil/pull/1604#discussion_r2692054582
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -63,13 +72,15 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
* Hence we are guaranteed to be properly aligned.
*/
final lazy val isKnownToBeAligned: Boolean =
LV(Symbol("isKnownToBeAligned")) {
- if (!isRepresented || (alignmentKindDefaulted == AlignmentKind.Manual))
true
- else {
- val pa = priorAlignmentWithLeadingSkipApprox
- val aa = alignmentApprox
- val res = (pa % aa) == 0
- res
- }
+ val res =
+ if (!isRepresented || (alignmentKindDefaulted == AlignmentKind.Manual))
true
+ else {
+ val pa = priorAlignmentWithLeadingSkipApprox
+ val aa = alignmentApprox
+ val res = (pa % aa) == 0
+ res
+ }
+ res
Review Comment:
Thoughts on having this else statement just end in
```
(pa % aa) == 0
```
Having the multiple res vals makes things a little confusing.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -222,75 +237,199 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
val priorAlignmentsApprox =
priorSibsAlignmentsApprox ++ parentAlignmentApprox ++
arraySelfAlignment ++ unorderedSequenceSelfAlignment
if (priorAlignmentsApprox.isEmpty)
- alignmentApprox // it will be the containing context's
responsibility to insure this IS where we start.
+ alignmentApprox // it will be the containing context's
responsibility to ensure this IS where we start.
else
priorAlignmentsApprox.reduce(_ * _)
}
}.value
- private lazy val priorAlignmentWithLeadingSkipApprox: AlignmentMultipleOf = {
- priorAlignmentApprox + leadingSkipApprox
+ private lazy val separatorPrefixInfixMTAApprox =
+ 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 =>
+ s.encodingLengthApprox
+ case _ => LengthExact(0)
}
- protected lazy val contentStartAlignment: AlignmentMultipleOf = {
- if (priorAlignmentWithLeadingSkipApprox % alignmentApprox == 0) {
- // alignment won't be needed, continue using prior alignment as start
alignment
- priorAlignmentWithLeadingSkipApprox
+ private lazy val initiatorMTAApprox =
+ if (this.hasInitiator) {
+ AlignmentMultipleOf(this.knownEncodingAlignmentInBits)
+ } else {
+ AlignmentMultipleOf(1)
+ }
+
+ private lazy val initiatorLengthApprox = if (this.hasInitiator) {
+ this.encodingLengthApprox
+ } else {
+ LengthExact(0)
+ }
+
+ private lazy val terminatorMTAApprox =
+ if (this.hasTerminator) {
+ AlignmentMultipleOf(this.knownEncodingAlignmentInBits)
} else {
- // alignment will be needed, it will be forced to be aligned to
alignmentApprox
- alignmentApprox
+ AlignmentMultipleOf(1)
+ }
+
+ private lazy val terminatorLengthApprox = if (this.hasTerminator) {
+ this.encodingLengthApprox
+ } else {
+ LengthExact(0)
+ }
+
+ /**
+ * prior alignment with leading skip includes the prefix/infix separator MTA
and length,
+ * any leading skips
+ */
+ private lazy val priorAlignmentWithLeadingSkipApprox: AlignmentMultipleOf = {
+ val priorAlignmentWithSeparatorApprox = priorAlignmentApprox
+ + separatorPrefixInfixMTAApprox
+ + separatorLengthApprox
+ val leadingAlignmentApprox = (priorAlignmentWithSeparatorApprox
+ + leadingSkipApprox)
+ leadingAlignmentApprox
+ }
+
+ /**
+ * 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
+ prefixTypeElem.elementSpecifiedLengthApprox
+ } else {
+ LengthExact(0)
+ }
+ }
+ case _ => LengthExact(0)
+ }
+ }
+
+ /**
+ * The alignment of the term's value.
+ *
+ * This is only relevant for simple elements with textual representation.
+ */
+ private lazy val valueMTAApprox = {
+ this match {
+ case eb: ElementBase if eb.isSimpleType && eb.representation ==
Representation.Text => {
+ AlignmentMultipleOf(eb.knownEncodingAlignmentInBits)
+ }
+ case _ => AlignmentMultipleOf(1)
}
}
- protected lazy val endingAlignmentApprox: AlignmentMultipleOf = {
+ /**
+ * This alignment is made up of the alignment of the term itself,
+ * the initiator MTA and length, the prefix length quasi
+ * element length, and the value MTA (we add it for optimization
+ * but is extremely difficult to test, as other things such
+ * as unparsers will provide MTA, even elementSpecifiedLengthApprox
+ * considers the encoding also).
+ */
+ protected lazy val contentStartAlignment: AlignmentMultipleOf = {
+ val leftFramingApprox = priorAlignmentWithLeadingSkipApprox
+ + alignmentApprox
+ + initiatorMTAApprox
+ + initiatorLengthApprox
+ val csa = (leftFramingApprox + prefixLengthElementLength) + valueMTAApprox
+ csa
+ }
+
+ /**
+ * Accounts for the start of the content start alignment and element length.
+ * This is used to determine the alignment before the terminator and
trailing skip
+ */
+ protected lazy val contentEndAlignment: AlignmentMultipleOf = {
this match {
case eb: ElementBase => {
- if (eb.isComplexType && eb.lengthKind == LengthKind.Implicit) {
- eb.complexType.group.endingAlignmentApprox + trailingSkipApprox
+ val res = if (eb.isComplexType && eb.lengthKind ==
LengthKind.Implicit) {
+ eb.complexType.group.contentEndAlignment
} else {
// simple type or complex type with specified length
- contentStartAlignment + (elementSpecifiedLengthApprox +
trailingSkipApprox)
+ contentStartAlignment + elementSpecifiedLengthApprox
}
+ res
}
case mg: ModelGroup => {
//
// We're interested in how the model group ends. Whatever could be
- // the last term, each individual such possibility has an
endingAlignmentApprox,
- // and we need to aggregate those to get the summary
endingAlignmentApprox.
+ // the last term, each individual such possibility has an
endingAlignmentWithRightFramingApprox,
+ // and we need to aggregate those to get the summary
endingAlignmentWithRightFramingApprox.
//
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.
- //
- val lceaa = lc.endingAlignmentApprox
- val res = lceaa + trailingSkipApprox
- res
+ // for each possible last child, gather its
endingAlignmentWithRightFramingApprox
+ // as we want to account for the separators that are a part of the
SequenceContent (see DFDL spec)
+ val lceaa = lc.endingAlignmentWithRightFramingApprox
+ 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(_ * _)
res
Review Comment:
Same here
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -222,75 +237,199 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
val priorAlignmentsApprox =
priorSibsAlignmentsApprox ++ parentAlignmentApprox ++
arraySelfAlignment ++ unorderedSequenceSelfAlignment
if (priorAlignmentsApprox.isEmpty)
- alignmentApprox // it will be the containing context's
responsibility to insure this IS where we start.
+ alignmentApprox // it will be the containing context's
responsibility to ensure this IS where we start.
else
priorAlignmentsApprox.reduce(_ * _)
}
}.value
- private lazy val priorAlignmentWithLeadingSkipApprox: AlignmentMultipleOf = {
- priorAlignmentApprox + leadingSkipApprox
+ private lazy val separatorPrefixInfixMTAApprox =
+ 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 =>
+ s.encodingLengthApprox
+ case _ => LengthExact(0)
}
- protected lazy val contentStartAlignment: AlignmentMultipleOf = {
- if (priorAlignmentWithLeadingSkipApprox % alignmentApprox == 0) {
- // alignment won't be needed, continue using prior alignment as start
alignment
- priorAlignmentWithLeadingSkipApprox
+ private lazy val initiatorMTAApprox =
+ if (this.hasInitiator) {
+ AlignmentMultipleOf(this.knownEncodingAlignmentInBits)
+ } else {
+ AlignmentMultipleOf(1)
+ }
+
+ private lazy val initiatorLengthApprox = if (this.hasInitiator) {
+ this.encodingLengthApprox
+ } else {
+ LengthExact(0)
+ }
+
+ private lazy val terminatorMTAApprox =
+ if (this.hasTerminator) {
+ AlignmentMultipleOf(this.knownEncodingAlignmentInBits)
} else {
- // alignment will be needed, it will be forced to be aligned to
alignmentApprox
- alignmentApprox
+ AlignmentMultipleOf(1)
+ }
+
+ private lazy val terminatorLengthApprox = if (this.hasTerminator) {
+ this.encodingLengthApprox
+ } else {
+ LengthExact(0)
+ }
+
+ /**
+ * prior alignment with leading skip includes the prefix/infix separator MTA
and length,
+ * any leading skips
+ */
+ private lazy val priorAlignmentWithLeadingSkipApprox: AlignmentMultipleOf = {
+ val priorAlignmentWithSeparatorApprox = priorAlignmentApprox
+ + separatorPrefixInfixMTAApprox
+ + separatorLengthApprox
+ val leadingAlignmentApprox = (priorAlignmentWithSeparatorApprox
+ + leadingSkipApprox)
+ leadingAlignmentApprox
+ }
+
+ /**
+ * 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
+ prefixTypeElem.elementSpecifiedLengthApprox
+ } else {
+ LengthExact(0)
+ }
+ }
+ case _ => LengthExact(0)
+ }
+ }
+
+ /**
+ * The alignment of the term's value.
+ *
+ * This is only relevant for simple elements with textual representation.
+ */
+ private lazy val valueMTAApprox = {
+ this match {
+ case eb: ElementBase if eb.isSimpleType && eb.representation ==
Representation.Text => {
+ AlignmentMultipleOf(eb.knownEncodingAlignmentInBits)
+ }
+ case _ => AlignmentMultipleOf(1)
}
}
- protected lazy val endingAlignmentApprox: AlignmentMultipleOf = {
+ /**
+ * This alignment is made up of the alignment of the term itself,
+ * the initiator MTA and length, the prefix length quasi
+ * element length, and the value MTA (we add it for optimization
+ * but is extremely difficult to test, as other things such
+ * as unparsers will provide MTA, even elementSpecifiedLengthApprox
+ * considers the encoding also).
+ */
+ protected lazy val contentStartAlignment: AlignmentMultipleOf = {
+ val leftFramingApprox = priorAlignmentWithLeadingSkipApprox
+ + alignmentApprox
+ + initiatorMTAApprox
+ + initiatorLengthApprox
+ val csa = (leftFramingApprox + prefixLengthElementLength) + valueMTAApprox
+ csa
+ }
+
+ /**
+ * Accounts for the start of the content start alignment and element length.
+ * This is used to determine the alignment before the terminator and
trailing skip
+ */
+ protected lazy val contentEndAlignment: AlignmentMultipleOf = {
this match {
case eb: ElementBase => {
- if (eb.isComplexType && eb.lengthKind == LengthKind.Implicit) {
- eb.complexType.group.endingAlignmentApprox + trailingSkipApprox
+ val res = if (eb.isComplexType && eb.lengthKind ==
LengthKind.Implicit) {
+ eb.complexType.group.contentEndAlignment
} else {
// simple type or complex type with specified length
- contentStartAlignment + (elementSpecifiedLengthApprox +
trailingSkipApprox)
+ contentStartAlignment + elementSpecifiedLengthApprox
}
+ res
}
case mg: ModelGroup => {
//
// We're interested in how the model group ends. Whatever could be
- // the last term, each individual such possibility has an
endingAlignmentApprox,
- // and we need to aggregate those to get the summary
endingAlignmentApprox.
+ // the last term, each individual such possibility has an
endingAlignmentWithRightFramingApprox,
+ // and we need to aggregate those to get the summary
endingAlignmentWithRightFramingApprox.
//
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.
- //
- val lceaa = lc.endingAlignmentApprox
- val res = lceaa + trailingSkipApprox
- res
+ // for each possible last child, gather its
endingAlignmentWithRightFramingApprox
+ // as we want to account for the separators that are a part of the
SequenceContent (see DFDL spec)
+ val lceaa = lc.endingAlignmentWithRightFramingApprox
+ 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
Review Comment:
Creating a separate val res for this seems unnecessary.
--
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]
