stevedlawrence commented on code in PR #1453:
URL: https://github.com/apache/daffodil/pull/1453#discussion_r1987745100
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/unparsers/UState.scala:
##########
@@ -126,10 +126,16 @@ abstract class UState(
val elt =
if (this.currentInfosetNodeMaybe.isDefined) "node=" +
this.currentInfosetNode.toString
else ""
- "UState(" + elt + " DOS=" + dataOutputStream.toString() + ")"
+ "UState(" + elt + " DOS=" + getDataOutputStream.toString() + ")"
}
- var dataOutputStream: DirectOrBufferedDataOutputStream
+ private var _dataOutputStream: DirectOrBufferedDataOutputStream = _
+
+ def getDataOutputStream = _dataOutputStream
+
+ def setDataOutputStream(dos: DirectOrBufferedDataOutputStream) = {
+ _dataOutputStream = dos
+ }
Review Comment:
Note that we don't necessarily need these getters/setters. We could make it
so dataOutputStream is still public and initialize it to `_`, and then instead
of overriding it, UStateForMain constructor would look like
```scala
dataOutputStream = DirectOrBufferedDataOutputStream(...)
```
and UStateForSuspension would look like
```scala
class UStateForSuspension(
...,
suspensionDataOutputStream: DirectOrBufferedDataOutputStream,
...) {
dataOutputStream = suspensionDataOutputSTream
}
```
That would avoid the needs for getters/setters and would make this a much
smaller change. Though it does make it a but unsafer since it requires
implementations to initialize it rather than the compiler forcing it to be
overridden and initialized, but it's not a big deal for us since we only have
these two classes.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/unparsers/UState.scala:
##########
@@ -423,6 +429,7 @@ final class UStateForSuspension(
areDebugging: Boolean
) extends UState(vbox, mainUState.diagnostics, mainUState.dataProc, tunable,
areDebugging) {
+ setDataOutputStream(dataOutputStream)
Review Comment:
This approach with getters/setters actually makes for a possible
enhancement. I *think* that UStateForSuspension should never change the
dataOutputStream. If that's true, you could change _dataOutputStream to
protected, and then this this could be changed to something like
```scala
_dataOutputStream = dataOutputSTream
```
And UStateForSuspension could overwrite setDataOutputStream to be something
like
```scala
override def setDataOutputStream(...) = {
Assert.invariantFailed("Should never change dataOutputStream on a
suspension")
}
```
This effecively forces the dataOutputStream field to be unchanging for
suspensions.
I don't think we necessarily need to do this as part of this PR (I'm not
even sure it suspension DOSs never change), but this is a potential advantage
for these kinds of setters/getters.
--
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]
