stevedlawrence commented on code in PR #1440:
URL: https://github.com/apache/daffodil/pull/1440#discussion_r1964382905
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/SequenceCombinator.scala:
##########
@@ -117,7 +117,11 @@ class OrderedSequence(sq: SequenceTermBase,
sequenceChildrenArg: Seq[SequenceChi
sepMtaAlignmentMaybe,
sepMtaUnparserMaybe,
sepUnparser,
- childUnparsers
+ childUnparsers.map(
+ _.asInstanceOf[
+ SequenceChildUnparser with
org.apache.daffodil.unparsers.runtime1.Separated
Review Comment:
Can `Separated` be imported so this can all fit on one line? Same with below
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceParserBases.scala:
##########
@@ -32,15 +31,13 @@ import org.apache.daffodil.runtime1.processors.Success
* Base class for all sequence parsers, which are the combinators that
coordinate
* all the parsing of the sequence child parsers.
*/
-abstract class SequenceParserBase(
+abstract class SequenceParserBase[T](
srd: SequenceRuntimeData,
- childParsers: Vector[Parser],
isOrdered: Boolean
) extends CombinatorParser(srd) {
override def nom = "Sequence"
- override lazy val runtimeDependencies: Vector[Evaluatable[AnyRef]] = Vector()
- override lazy val childProcessors = childParsers
+ def childParsers: Array[T]
Review Comment:
This might want to be a val? That would ensure it is impossible for sub
classes to override this with a function that might be expensive to evaluate. I
think we currently always override it with a val, but that would enforce it.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceParserBases.scala:
##########
@@ -32,15 +31,13 @@ import org.apache.daffodil.runtime1.processors.Success
* Base class for all sequence parsers, which are the combinators that
coordinate
* all the parsing of the sequence child parsers.
*/
-abstract class SequenceParserBase(
+abstract class SequenceParserBase[T](
Review Comment:
It looks like all uses of `SequenceParserBase` use the same
`SequenceChildParser` as the type parameter, so the type parameter isn't really
adding much. Is it possible to remove this and change childParsers to
`Array[SequenceParserBase]`?
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -122,7 +122,7 @@ class BinaryIntegerRuntimeLengthUnparser(
) extends BinaryIntegerBaseUnparser(e)
with HasRuntimeExplicitLength {
- override val runtimeDependencies = Vector(lengthEv)
+ override def runtimeDependencies = Vector(lengthEv)
Review Comment:
There's still a number of `runtimeDependencies` that are vals:
```
$ grep -R 'val runtimeDependencies' | wc -l
88
```
Should those be changed to a def too?
--
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]
