Add Maybe[T].toArray and fix DelimiterTextParser array aliasing - #1720
Conversation
stevedlawrence
left a comment
stevedlawrence
left a comment
There was a problem hiding this comment.
+1
Good cleanup, but probably won't help with runtime performance much since runtimeDependencies are all calculated at compile time
| // .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() |
There was a problem hiding this 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.
4276f0e to
14f0ca7
Compare
Added Maybe[T].toArray, replacing seven hand-rolled .toList.toArray call sites across the runtimeDependencies migration to Array (EvElement, EvEscapeSchemes, EvFieldDFA, StringLiteralForUnparser, SpecifiedLength2, ElementUnparser x2) with a single reusable method. DAFFODIL-3065
14f0ca7 to
d0ce3ae
Compare
3 participants
Added Maybe[T].toArray, replacing seven hand-rolled .toList.toArray call sites across the runtimeDependencies migration to Array (EvElement, EvEscapeSchemes, EvFieldDFA, StringLiteralForUnparser, SpecifiedLength2, ElementUnparser x2) with a single reusable method.
DelimiterTextParser.runtimeDependencies now .clone()s EncodingRuntimeData's array instead of aliasing the same mutable instance: Array is invariant and mutable, unlike the Vector this used to be, so sharing the identical instance would let a future in-place mutation (e.g. dedupe or append) silently corrupt every sibling delimiter parser's dependency list along with EncodingRuntimeData's own.
DAFFODIL-3065