stevedlawrence commented on a change in pull request #448:
URL: https://github.com/apache/incubator-daffodil/pull/448#discussion_r512633822
##########
File path:
daffodil-io/src/main/scala/org/apache/daffodil/io/DirectOrBufferedDataOutputStream.scala
##########
@@ -885,8 +885,10 @@ class DirectOrBufferedDataOutputStream private[io] (
bufOS.getFile.delete()
}
nBitsPut / 8
- } else
- directDOS.putBytes(bufOS.getBuf, 0, nBytes, finfo)
+ } else {
+ Assert.usage(nBytes <= Int.MaxValue)
Review comment:
I think this shoudl be an ``Assert.invariant``? My understanding is that
``Assert.usage`` has the implication that we're just not following our own
assumptions about a function or something. For example, a function can only be
called if isError is false, so we might have ``Assert.usage(!isError)``. It's
still possible to call a function when things are an error, but we shouldn't do
that.
But ``Assert.invariant`` implies that it shouldn't even be possible, not
matter what we do. In this case, the size of a non-file backed buffer should
never be larger than IntMax because a non-file backed buffer is backed by an
ArrayBufffer, which can never be larger than IntMax. So nBytes being larger
than IntMax should be impossible.
Also, below this in the else block which I think starts with the exact same
code with just different variable names (and missing the new Assert.usage
you've added). While we are here, I wonder it it makes sense to move this chunk
of code that writes whole bytes out of the if-else blocks and have it always
run so we remove this duplicate logic. Then there's just the if-statement
forhandling fragment bytes if they exist?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
