stevedlawrence commented on code in PR #1720:
URL: https://github.com/apache/daffodil/pull/1720#discussion_r3856578440
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/DelimiterParsers.scala:
##########
@@ -51,7 +51,10 @@ class DelimiterTextParser(
mustMatchNonZeroData: Boolean
) extends TextPrimParser {
- override val runtimeDependencies = rd.encodingInfo.runtimeDependencies
+ // .clone(), not the same reference: Array is mutable, unlike the
+ // Vector this used to be, so sharing the instance would let a future
+ // in-place mutation corrupt every sibling delimiter parser's list too.
+ override val runtimeDependencies =
rd.encodingInfo.runtimeDependencies.clone()
Review Comment:
I don't think this clone is really necessary. Although runtimeDependencies
are mutable Arrays, I don't think we ever actually mutate them.
And even if encodingInfo.runtimeDependencies did change, it's probably more
correct for this runtimeDependencies to also update, so there's an argument
that cloning is actually the wrong behavior. But we don't have that behavior
for other runtimeDependnecies, so I don't think we're really consistent with
that behavior.
If we wanted to ensure it's never mutated we could change the type to Scala
3's `IArray` (immutable array), but I'm not sure if the churn is worth it. It's
not too hard to inspect all the runtimeDependnecies and verify they never
change.
--
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]
