stevedlawrence commented on code in PR #1594:
URL: https://github.com/apache/daffodil/pull/1594#discussion_r2559935102


##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -228,8 +233,53 @@ 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 => 
LengthMultipleOf(s.knownEncodingAlignmentInBits)
+          case Postfix => LengthMultipleOf(0)
+        }
+      case _ => LengthMultipleOf(0)
+    }
+
+  private lazy val separatorPostfixMTAApprox =
+    this.optLexicalParent match {
+      case Some(s: SequenceTermBase) if s.hasSeparator =>
+        import SeparatorPosition.*
+        s.separatorPosition match {
+          case Prefix | Infix => LengthMultipleOf(0)
+          case Postfix => LengthMultipleOf(s.knownEncodingAlignmentInBits)
+        }
+      case _ => LengthMultipleOf(0)
+    }
+
+  private lazy val separatorLengthApprox = this.optLexicalParent match {
+    case Some(s: SequenceTermBase) if s.hasSeparator =>
+      getEncodingLengthApprox(s)
+    case _ => LengthMultipleOf(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)
+    }
+  }
+
   private lazy val priorAlignmentWithLeadingSkipApprox: AlignmentMultipleOf = {

Review Comment:
   Maybe we should rename this, since it's now a bit more than just prior 
alignment with leading skip.  I *think* it's correct to all separators, leading 
skip, initiators (not currently implemented), etc. as "framing"? So maybe this 
wants to be `priorAlignmentWithLeftFramingApprox`?



##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -242,15 +292,24 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
     }
   }
 
+  /**
+   * The postfix separator MTA/length needs to be added after the trailing skip
+   *
+   * TODO: DAFFODIL-3057 needs to consider terminator MTA/length before 
trailingSkip
+   */
   protected lazy val endingAlignmentApprox: AlignmentMultipleOf = {
     this match {
       case eb: ElementBase => {
-        if (eb.isComplexType && eb.lengthKind == LengthKind.Implicit) {
+        val ea = if (eb.isComplexType && eb.lengthKind == LengthKind.Implicit) 
{
           eb.complexType.group.endingAlignmentApprox + trailingSkipApprox
         } else {
           // simple type or complex type with specified length
           contentStartAlignment + (elementSpecifiedLengthApprox + 
trailingSkipApprox)
         }
+        val endingAlignmentWithSeparatorApprox = ea
+          + separatorPostfixMTAApprox
+          + separatorLengthApprox
+        endingAlignmentWithSeparatorApprox
       }
       case mg: ModelGroup => {

Review Comment:
   I believe model groups can also have postfix separators if the model group 
is a child of a separated sequence, so the above logic can't just be for 
`ElementBase`.
   
   Does the postifx separator mta and length always needs to be added to the 
result of the match case? So where this thing ends (whether it be an element or 
a model group) we add the approximate alignment?



-- 
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]

Reply via email to