stevedlawrence commented on code in PR #988:
URL: https://github.com/apache/daffodil/pull/988#discussion_r1143268255
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/Parser.scala:
##########
@@ -65,12 +66,51 @@ sealed trait Parser extends Processor {
pstate.setFailed(new ParseError(One(sfl), One(dataLoc), s, args: _*))
}
- def PENotEnoughBits(pstate: PState, neededBits: Long, remainingBits:
MaybeULong) = {
+ def PENotEnoughBits(
+ pstate: PState,
+ neededBits: Long,
+ startPos: Long,
+ source: InputSourceDataInputStream,
+ ): Unit = {
+ val remainingLimitedBits = {
+ if (source.bitLimit0b.isEmpty) MaybeULong.Nope
+ else {
+ val lim = source.bitLimit0b.get
+ Assert.invariant(lim >= 0)
+ val nBits = lim - startPos
+ MaybeULong(nBits)
+ }
+ }
+
+ val remainingBits = {
+ if (source.hasReachedEndOfData) {
+ val bitsAvailable = {
+ if (startPos % 8 == 0)
+ source.knownBytesAvailable * 8
+ else {
+ source.knownBytesAvailable * 8 - (startPos % 8)
+ }
+ }
+ if (remainingLimitedBits.isEmpty || bitsAvailable <
remainingLimitedBits.get)
+ MaybeULong(bitsAvailable)
+ else
+ remainingLimitedBits
+ } else remainingLimitedBits
+ }
+
val remainingStr =
if (remainingBits.isDefined) s" but found only ${remainingBits.get}
available" else ""
PE(pstate, "Insufficient bits in data. Needed %d bit(s)%s.", neededBits,
remainingStr)
}
+ def PENotEnoughBits(
+ pstate: PState,
+ neededBits: Long,
+ source: InputSourceDataInputStream,
+ ): Unit = {
+ PENotEnoughBits(pstate, neededBits, source.bitPos0b, source)
+ }
+
def PENotEnoughBits(
Review Comment:
I'd suggest the blobs should use this `PENotEnoughBits` (and modify it to
have the remaining bits logic) and pass in a `DataLocation` saved from the
beginning of the blob parsing before it started reading chunks. And then the
remaining bits logic could adjust bits based on the difference between
`DataLocation.bitPos1b - 1` and `pstate.bitPos0b`. We'll also want to modify
this to call the `PE` function that accepts a `DataLocation` so the
`ParseError` uses the saved `DataLocation` instead of the one from the
`pstate`. This way the generated diagnostics has he correct bitPosition at the
beginning of the blob.
The above `PENotENoughBits` that everything else uses can then just call
this one, passing in `pstate.currentLocation` as the `DataLocation`. In that
case the difference between `DataLocation.bitPos1b - 1` and `pstate.bitPos0b`
will be zero, so the adjusting logic just won't change anything.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/Parser.scala:
##########
@@ -65,12 +66,51 @@ sealed trait Parser extends Processor {
pstate.setFailed(new ParseError(One(sfl), One(dataLoc), s, args: _*))
}
- def PENotEnoughBits(pstate: PState, neededBits: Long, remainingBits:
MaybeULong) = {
+ def PENotEnoughBits(
+ pstate: PState,
+ neededBits: Long,
+ startPos: Long,
+ source: InputSourceDataInputStream,
+ ): Unit = {
+ val remainingLimitedBits = {
+ if (source.bitLimit0b.isEmpty) MaybeULong.Nope
+ else {
+ val lim = source.bitLimit0b.get
+ Assert.invariant(lim >= 0)
+ val nBits = lim - startPos
+ MaybeULong(nBits)
+ }
+ }
+
+ val remainingBits = {
+ if (source.hasReachedEndOfData) {
+ val bitsAvailable = {
+ if (startPos % 8 == 0)
+ source.knownBytesAvailable * 8
+ else {
+ source.knownBytesAvailable * 8 - (startPos % 8)
+ }
+ }
Review Comment:
I personally find this hard to read. I think it would be more clear to split
it up, something like:
```scala
val fragmentBitsRead = startPos % 8
val bitsAvailable = (source.knownBytesAvailable * 8) - fragmentBitsRead
```
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/Parser.scala:
##########
@@ -65,12 +66,51 @@ sealed trait Parser extends Processor {
pstate.setFailed(new ParseError(One(sfl), One(dataLoc), s, args: _*))
}
- def PENotEnoughBits(pstate: PState, neededBits: Long, remainingBits:
MaybeULong) = {
+ def PENotEnoughBits(
+ pstate: PState,
+ neededBits: Long,
+ startPos: Long,
+ source: InputSourceDataInputStream,
+ ): Unit = {
+ val remainingLimitedBits = {
+ if (source.bitLimit0b.isEmpty) MaybeULong.Nope
+ else {
+ val lim = source.bitLimit0b.get
+ Assert.invariant(lim >= 0)
+ val nBits = lim - startPos
+ MaybeULong(nBits)
+ }
+ }
+
+ val remainingBits = {
+ if (source.hasReachedEndOfData) {
+ val bitsAvailable = {
+ if (startPos % 8 == 0)
+ source.knownBytesAvailable * 8
+ else {
+ source.knownBytesAvailable * 8 - (startPos % 8)
+ }
+ }
+ if (remainingLimitedBits.isEmpty || bitsAvailable <
remainingLimitedBits.get)
+ MaybeULong(bitsAvailable)
+ else
+ remainingLimitedBits
+ } else remainingLimitedBits
+ }
+
val remainingStr =
if (remainingBits.isDefined) s" but found only ${remainingBits.get}
available" else ""
PE(pstate, "Insufficient bits in data. Needed %d bit(s)%s.", neededBits,
remainingStr)
Review Comment:
I think this still suffers from a previous comment. If data was chunked,
this call to `PE` will use `DataLocation` from the `pstate`, which could be too
far forward in the data (i.e. where the last successful chunk finished,
instead of where the blob started actually reading). And `reaminigStr` is going
to only say the amount of bits remaining after the last successful chunk was
read.
--
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]
