stevedlawrence commented on code in PR #1601:
URL: https://github.com/apache/daffodil/pull/1601#discussion_r2619743910
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -232,6 +235,31 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
priorAlignmentApprox + leadingSkipApprox
}
+ private lazy val terminatorMTAApprox =
+ this match {
+ case t if t.hasTerminator =>
+ LengthMultipleOf(t.knownEncodingAlignmentInBits)
+ case _ => LengthMultipleOf(0)
+ }
+
+ private lazy val terminatorLengthApprox = this match {
+ case t if t.hasTerminator =>
+ getEncodingLengthApprox(t)
+ case _ => LengthMultipleOf(0)
+ }
Review Comment:
Same here, can this be simplified to an if-statement?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -266,21 +304,30 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
// it the actual last child.
//
val lceaa = lc.endingAlignmentApprox
- val res = lceaa + trailingSkipApprox
+ val res = lceaa
+ + terminatorMTAApprox
+ + terminatorLengthApprox
+ + trailingSkipApprox
res
}
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.
//
- Some(contentStartAlignment + trailingSkipApprox)
- else
+ val res = Some(
+ contentStartAlignment
+ + terminatorMTAApprox
+ + terminatorLengthApprox
+ + trailingSkipApprox
+ )
+ res
+ } else
Review Comment:
Instead of adding the same three fields to a bunch of places in the various
conditions can we do something like
```scala
val contentEndAlignment = ... //existing logic
val res =
contentEndAlignment
+ terminatorMTAAPprox
+ terminatorLengthApprox
+ trailingSkipApprox
```
I'm not sure if contentEndAlignment is the right name, but something along
those lines?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -232,6 +235,31 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
priorAlignmentApprox + leadingSkipApprox
}
+ private lazy val terminatorMTAApprox =
+ this match {
+ case t if t.hasTerminator =>
+ LengthMultipleOf(t.knownEncodingAlignmentInBits)
+ case _ => LengthMultipleOf(0)
Review Comment:
I think these want to be `AlignmentMultipleOf` since they represent an
alignment and not a length. I imagine the math ends up the same. Though maybe
the default case then becomes AlignmentMultipleOf(1)?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -232,6 +235,31 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
priorAlignmentApprox + leadingSkipApprox
}
+ private lazy val terminatorMTAApprox =
+ this match {
Review Comment:
Can this match case be simplified to an if-statement?
--
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]
