[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #988: Update parse error message

["stevedlawrence (via GitHub)" <gi...@apache.org>]

stevedlawrence commented on code in PR #988:
URL: https://github.com/apache/daffodil/pull/988#discussion_r1138621563


##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/BlobLengthParsers.scala:
##########
@@ -55,9 +55,9 @@ trait ByteChunkWriter { self: PrimParser =>
         remainingBitsToGet -= bitsToGet
       } else {
         val remainingBits =
-          if (dis.remainingBits.isDefined) {
+          if (dis.remainingLimitedBits.isDefined) {
             val totalBitsRead = nBits - remainingBitsToGet
-            MaybeULong(dis.remainingBits.get + totalBitsRead)
+            MaybeULong(dis.remainingLimitedBits.get + totalBitsRead)

Review Comment:
   Yeah, I think this might actually be incorrect? This use of `remainingBits` here is to calculate a parameter for `PENotEnoughBits`. And since this is for an error message, like the other uses of `remainingBits`, I think we do want to take into account EOF, and so should use `remainingBits`.
   
   Which I think means there are no more uses of `remainingLimitedBits`, and maybe it's logic can just be merge into `remainingBits`?
   
   Another thought, nothing should really use `reaminingBits` to make parsing decision (we don't currently, but we could accidentally in the future). Parsers should always use `isDefinedForLength` if they want to know if bits are available, since that takes into account both bit limit and EOF, and will also read and cache bytes from the input stream as needed. The only place it is reasonable to use is in error messages when `isDefinedForLength` returns false, which is how it's used right now (and it's not used in unparse at all). Since it really shouldn't be used, and is only used for not enough bits diagnostics, I wonder if we can just remove it and move the logic into `PENotEnoughBits` available, or at least rename it so it's clear it should only be used for diagnostics when `isDefinedForLength` fails? This would avoid us accidentally using it for actual parsing decisions.



-- 
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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org