stevedlawrence commented on code in PR #1337:
URL: https://github.com/apache/daffodil/pull/1337#discussion_r1797151095
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/BinaryNumberParsers.scala:
##########
@@ -166,7 +166,22 @@ abstract class BinaryIntegerBaseParser(
def parse(start: PState): Unit = {
val nBits = getBitLength(start)
- if (nBits == 0) return // zero length is used for outputValueCalc often.
+ // minimum length for a signed binary integer is 2 bits, for unsigned it
is 1 bit
+ if (signed && nBits < 2) {
+ PE(
+ start,
+ "Minimum length for a signed binary integer is 2 bits, number of bits
%d out of range.",
+ nBits
+ )
+ return
+ } else if (!signed && nBits < 1) {
+ PE(
+ start,
+ "Minimum length for an unsigned binary integer is 1 bit, number of
bits %d out of range.",
+ nBits
+ )
+ return
+ }
Review Comment:
I think we also need these checks at compile time for non-expression
explicit lengths and it wants to be an SDE? See `maybeFixedLengthInBits`
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/BinaryNumberParsers.scala:
##########
@@ -166,7 +166,22 @@ abstract class BinaryIntegerBaseParser(
def parse(start: PState): Unit = {
val nBits = getBitLength(start)
- if (nBits == 0) return // zero length is used for outputValueCalc often.
+ // minimum length for a signed binary integer is 2 bits, for unsigned it
is 1 bit
+ if (signed && nBits < 2) {
+ PE(
+ start,
+ "Minimum length for a signed binary integer is 2 bits, number of bits
%d out of range.",
+ nBits
+ )
+ return
+ } else if (!signed && nBits < 1) {
+ PE(
+ start,
+ "Minimum length for an unsigned binary integer is 1 bit, number of
bits %d out of range.",
+ nBits
+ )
+ return
+ }
if (primNumeric.width.isDefined) {
val width = primNumeric.width.get
if (nBits > width)
Review Comment:
Just below is a PE without a return, which I think is a bug? According to
codecov we already have coverage for this, so I imagine the missing return just
means we do extra unnecessary work to parse a number that is too big and then
just ignore it. Can you confirm if this is needed and open a bug if so?
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -93,6 +93,27 @@ abstract class BinaryIntegerBaseUnparser(e:
ElementRuntimeData, signed: Boolean)
dos.putLong(asLong(value), nBits, finfo)
}
}
+
+ override def unparse(state: UState): Unit = {
+ val nBits = getBitLength(state)
Review Comment:
This is going to call getBitLength twice, once here and once when super is
called. We should probably avoid that.
Also, it looks like we don't have any checks on the max size for numbers
like we do in parse. Maybe we should just do all the min/max length checks in
the main unparse function like we do with parse? Or maybe in
BinaryIntegerBaseUnparser.putNumber?
##########
daffodil-test/src/test/resources/org/apache/daffodil/extensions/enum/enums.tdml:
##########
@@ -134,7 +134,7 @@
</element>
<simpleType name="myBit" dfdl:lengthKind="explicit" dfdl:length="1">
- <restriction base="xs:byte"/>
+ <restriction base="xs:unsignedByte"/>
Review Comment:
I'm concerned we will have a lot of regressions due to this change, and
wonder if we need a process of deprecating this via tunable. It might be worth
running this change with the regression suite and seeing how many things rely
on this behavior.
--
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]
