stevedlawrence commented on code in PR #1337:
URL: https://github.com/apache/daffodil/pull/1337#discussion_r1808727010
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -775,32 +775,36 @@ trait ElementBase
result.isDefined && repElement.isSimpleType && representation ==
Representation.Binary
) {
val nBits = result.get
- if (isSignedIntegerType && nBits < 2) {
- val outOfRangeStr =
- "Minimum length for a signed binary integer is 2 bits, number of
bits %d out of range. " +
- "An unsigned integer with length 1 bit could be used instead."
- if (tunable.allowSignedIntegerLength1Bit) {
- SDW(
- WarnID.SignedBinaryIntegerLength1Bit,
- outOfRangeStr,
- nBits
- )
- } else {
- SDE(
- outOfRangeStr,
- nBits
- )
- }
- }
- schemaDefinitionWhen(
- isUnsignedIntegerType && nBits < 1,
- "Minimum length for an unsigned binary integer is 1 bit, number of
bits %d out of range.",
- nBits
- )
primType match {
case primNumeric: NodeInfo.PrimType.PrimNumeric =>
- if (primNumeric.width.isDefined) {
- val width = primNumeric.width.get
+ if (primNumeric.minWidth.isDefined) {
+ val isSigned = primNumeric.isSigned
+ val signedStr = if (isSigned) "signed" else "unsigned"
Review Comment:
isSigned and signedStr aren't used unless we fall into the nBits < minWidth
block. Suggest we move this val into that block.
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/generators/BinaryIntegerKnownLengthCodeGenerator.scala:
##########
@@ -35,6 +35,10 @@ trait BinaryIntegerKnownLengthCodeGenerator extends
BinaryValueCodeGenerator {
case n if n <= 64 => 64
case _ => e.SDE("Binary integer lengths longer than 64 bits are not
supported.")
}
+ val signed = e.primType match {
+ case n: PrimType.PrimNumeric => n.isSigned
+ case _ => false
Review Comment:
Seems this function is only use for numerics, this probably wants to be an
assert.impossible with code coverage disabed, or just do
`e.primType.asInstanceOf[PrimNumeric].isSigned`
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -78,17 +76,61 @@ abstract class BinaryNumberBaseUnparser(override val
context: ElementRuntimeData
}
-abstract class BinaryIntegerBaseUnparser(e: ElementRuntimeData, signed:
Boolean)
+abstract class BinaryIntegerBaseUnparser(e: ElementRuntimeData)
extends BinaryNumberBaseUnparser(e) {
+ private val primNumeric =
e.optPrimType.get.asInstanceOf[NodeInfo.PrimType.PrimNumeric]
+
override def putNumber(
dos: DataOutputStream,
value: JNumber,
nBits: Int,
finfo: FormatInfo
): Boolean = {
+ val state = finfo.asInstanceOf[UState]
+ if (primNumeric.minWidth.isDefined) {
+ val isSigned = primNumeric.isSigned
+ val signedStr = if (isSigned) "signed" else "unsigned"
Review Comment:
Same as above, suggest moving vals into the block where they are used.
Avoids unnecessary work (though in this case the work is pretty minimal).
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -180,7 +225,11 @@ class BinaryDoubleUnparser(e: ElementRuntimeData) extends
BinaryNumberBaseUnpars
nBits: Int,
finfo: FormatInfo
): Boolean = {
- dos.putBinaryDouble(asDouble(value), finfo)
+ if (nBits > 0) {
+ dos.putBinaryDouble(asDouble(value), finfo)
Review Comment:
Same as the float case, but it's always 64 bits.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -268,6 +317,10 @@ abstract class BinaryDecimalUnparserBase(
nBits: Int,
finfo: FormatInfo
): Boolean = {
- dos.putBigInt(asBigInt(value), nBits, signed == YesNo.Yes, finfo)
+ if (nBits > 0) {
+ dos.putBigInt(asBigInt(value), nBits, signed == YesNo.Yes, finfo)
Review Comment:
Does BinaryDecimal have the same restriction about signed/unsigned and
1-bit/2-bit? For example, if `signed == Yes` and `nBits == 1`, should that be a
warning or UE depending on the tunable value?
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -268,6 +317,10 @@ abstract class BinaryDecimalUnparserBase(
nBits: Int,
finfo: FormatInfo
): Boolean = {
- dos.putBigInt(asBigInt(value), nBits, signed == YesNo.Yes, finfo)
+ if (nBits > 0) {
+ dos.putBigInt(asBigInt(value), nBits, signed == YesNo.Yes, finfo)
+ } else {
+ true
Review Comment:
Should this be `false`? I think the purpose of the bug is to make it change
it so zero-length integers are now an error? If you put the nBits == 0 check in
the `unparse` method we can handle this case for all binary numbers.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -56,12 +58,8 @@ abstract class BinaryNumberBaseUnparser(override val
context: ElementRuntimeData
val nBits = getBitLength(state)
val value = getNumberToPut(state)
val dos = state.dataOutputStream
- val res =
- if (nBits > 0) {
- putNumber(dos, value, nBits, state)
- } else {
- true
- }
+ val res = putNumber(dos, value, nBits, state)
Review Comment:
Seems this should be
```scala
if (nBits == 0) UE(...)
else putNumber(...)
```
I think the original purpose of this bug was to make it so a length of 0
changes from being ignored to being an error. Does it make sense to handle the
zero case here and handle the min/maxWidth checks in putNumber, since those are
specific to the number type?
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -78,17 +76,61 @@ abstract class BinaryNumberBaseUnparser(override val
context: ElementRuntimeData
}
-abstract class BinaryIntegerBaseUnparser(e: ElementRuntimeData, signed:
Boolean)
+abstract class BinaryIntegerBaseUnparser(e: ElementRuntimeData)
extends BinaryNumberBaseUnparser(e) {
+ private val primNumeric =
e.optPrimType.get.asInstanceOf[NodeInfo.PrimType.PrimNumeric]
+
override def putNumber(
dos: DataOutputStream,
value: JNumber,
nBits: Int,
finfo: FormatInfo
): Boolean = {
+ val state = finfo.asInstanceOf[UState]
+ if (primNumeric.minWidth.isDefined) {
+ val isSigned = primNumeric.isSigned
+ val signedStr = if (isSigned) "signed" else "unsigned"
+ val minWidth = primNumeric.minWidth.get
+ if(nBits < minWidth) {
+ val outOfRangeFmtStr =
+ "Minimum length for a %s binary integer is %d bit(s), number of bits
%d out of range. " +
+ "An unsigned integer with length 1 bit could be used instead."
+ if (isSigned && state.tunable.allowSignedIntegerLength1Bit) {
+ state.SDW(
+ WarnID.SignedBinaryIntegerLength1Bit,
+ outOfRangeFmtStr,
+ signedStr,
+ minWidth,
+ nBits
+ )
+ return false
+ } else {
+ UE(
+ state,
+ outOfRangeFmtStr,
+ signedStr,
+ minWidth,
+ nBits
+ )
+ return false
Review Comment:
`return false` is unnecessary, UE throws an exception and bubbles up to the
top to end the unparse.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -893,7 +892,7 @@ trait ElementBaseGrammarMixin
private lazy val packedSignCodes =
PackedSignCodes(binaryPackedSignCodes, binaryNumberCheckPolicy)
- private def binaryIntegerValue(isSigned: Boolean) = {
+ private def binaryIntegerValue = {
Review Comment:
Does this just want to be a `lazy val` now that it isn't parameterized?
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -78,17 +76,61 @@ abstract class BinaryNumberBaseUnparser(override val
context: ElementRuntimeData
}
-abstract class BinaryIntegerBaseUnparser(e: ElementRuntimeData, signed:
Boolean)
+abstract class BinaryIntegerBaseUnparser(e: ElementRuntimeData)
extends BinaryNumberBaseUnparser(e) {
+ private val primNumeric =
e.optPrimType.get.asInstanceOf[NodeInfo.PrimType.PrimNumeric]
+
override def putNumber(
dos: DataOutputStream,
value: JNumber,
nBits: Int,
finfo: FormatInfo
): Boolean = {
+ val state = finfo.asInstanceOf[UState]
+ if (primNumeric.minWidth.isDefined) {
+ val isSigned = primNumeric.isSigned
+ val signedStr = if (isSigned) "signed" else "unsigned"
+ val minWidth = primNumeric.minWidth.get
+ if(nBits < minWidth) {
+ val outOfRangeFmtStr =
+ "Minimum length for a %s binary integer is %d bit(s), number of bits
%d out of range. " +
+ "An unsigned integer with length 1 bit could be used instead."
+ if (isSigned && state.tunable.allowSignedIntegerLength1Bit) {
+ state.SDW(
+ WarnID.SignedBinaryIntegerLength1Bit,
+ outOfRangeFmtStr,
+ signedStr,
+ minWidth,
+ nBits
+ )
+ return false
Review Comment:
Is this `return false` correct? If we are only warning then should we still
unparse something? For example, if nBits is 1 and we are signed but
allowSignedIntegerLength1Bit, then we should output 1 bit. Seems like we want
to warn but then drop through and still put a number.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -163,7 +204,11 @@ class BinaryFloatUnparser(e: ElementRuntimeData) extends
BinaryNumberBaseUnparse
nBits: Int,
finfo: FormatInfo
): Boolean = {
- dos.putBinaryFloat(asFloat(value), finfo)
+ if (nBits > 0) {
Review Comment:
I believe binary float must be exactly 32 bits, anything else should be an
error. In fact, I think we have a compile time restriction the binary float
must have a known compile time length of 32 bits or its an SDE, so I think it's
impossible for nBits to not be correct and we should just always call
putBinaryFloat.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/BinaryNumberParsers.scala:
##########
@@ -166,9 +163,36 @@ abstract class BinaryIntegerBaseParser(
def parse(start: PState): Unit = {
val nBits = getBitLength(start)
- if (nBits == 0) return // zero length is used for outputValueCalc often.
- if (primNumeric.width.isDefined) {
- val width = primNumeric.width.get
+ if (primNumeric.minWidth.isDefined) {
+ val isSigned = primNumeric.isSigned
+ val signedStr = if (isSigned) "signed" else "unsigned"
+ val minWidth = primNumeric.minWidth.get
+ if(nBits < minWidth) {
+ val outOfRangeFmtStr =
+ "Minimum length for a %s binary integer is %d bit(s), number of bits
%d out of range. " +
+ "An unsigned integer with length 1 bit could be used instead."
+ if (isSigned && start.tunable.allowSignedIntegerLength1Bit) {
+ start.SDW(
+ WarnID.SignedBinaryIntegerLength1Bit,
+ outOfRangeFmtStr,
+ signedStr,
+ minWidth,
+ nBits
+ )
+ } else {
+ PE(
+ start,
+ outOfRangeFmtStr,
+ signedStr,
+ minWidth,
+ nBits
+ )
+ return
Review Comment:
We should probably have test for the missing code coverage.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala:
##########
@@ -70,11 +71,16 @@ abstract class PackedBinaryDecimalBaseParser(
abstract class PackedBinaryIntegerBaseParser(
override val context: ElementRuntimeData,
- signed: Boolean = false
) extends PrimParser
with PackedBinaryConversion {
override lazy val runtimeDependencies = Vector()
+ val signed = {
+ context.optPrimType.get match {
+ case n: NodeInfo.PrimType.PrimNumeric => n.isSigned
+ case _ => false
+ }
+ }
protected def getBitLength(s: ParseOrUnparseState): Int
def parse(start: PState): Unit = {
Review Comment:
Just below we have this line:
```scala
if (nBits == 0) return // zero length is used for outputValueCalc often.
```
The purpose of this bug was to make it so zero length numbers is an error. I
don't understand why outputValueCalc would have zero length, but based on the
bug it sounds like that shouldn't be allowed any more. Should that change to an
unparse error?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -686,8 +692,9 @@ object NodeInfo extends Enum {
case _ => true
}
}
-
- override val width: MaybeInt = MaybeInt.Nope
+ override val isSigned: Boolean = true
+ override val minWidth: MaybeInt = MaybeInt(2)
Review Comment:
Decimal is a bit of a weird case. It can be either signed or unsigned
depending on the `dfdl:decimalSigned` property. And if that is false, then
minWidth could be 1. Maybe we change this to a Nope to make it clear asking the
minWidth of decimal doesn't really make sense, and you need to special case
width checking fro decimals?
And actually, the spec says that the minimum width for xs:decimal is 8 bits,
because lengthUnits="bits" is not allowed for xs:decimal. But we've removed
that restriction for all the types so maybe this doesn't apply for us.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -534,7 +534,9 @@ object NodeInfo extends Enum {
}
trait PrimNumeric { self: Numeric.Kind =>
- def width: MaybeInt
+ val isSigned: Boolean
Review Comment:
Make this a def. Def's can be overridden with a val, but not the other way
around, so def is a bit more flexible for implementations.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -767,7 +781,9 @@ object NodeInfo extends Enum {
protected override def fromNumberNoCheck(n: Number): DataValueByte =
n.byteValue
override val min = JByte.MIN_VALUE.toLong
override val max = JByte.MAX_VALUE.toLong
- override val width: MaybeInt = MaybeInt(8)
+ override val isSigned: Boolean = true
+ override val minWidth: MaybeInt = MaybeInt(1)
Review Comment:
Byte is signed so I think minWidth should be 2?
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -78,17 +76,61 @@ abstract class BinaryNumberBaseUnparser(override val
context: ElementRuntimeData
}
-abstract class BinaryIntegerBaseUnparser(e: ElementRuntimeData, signed:
Boolean)
+abstract class BinaryIntegerBaseUnparser(e: ElementRuntimeData)
extends BinaryNumberBaseUnparser(e) {
+ private val primNumeric =
e.optPrimType.get.asInstanceOf[NodeInfo.PrimType.PrimNumeric]
+
override def putNumber(
dos: DataOutputStream,
value: JNumber,
nBits: Int,
finfo: FormatInfo
): Boolean = {
+ val state = finfo.asInstanceOf[UState]
+ if (primNumeric.minWidth.isDefined) {
+ val isSigned = primNumeric.isSigned
+ val signedStr = if (isSigned) "signed" else "unsigned"
+ val minWidth = primNumeric.minWidth.get
+ if(nBits < minWidth) {
+ val outOfRangeFmtStr =
+ "Minimum length for a %s binary integer is %d bit(s), number of bits
%d out of range. " +
+ "An unsigned integer with length 1 bit could be used instead."
+ if (isSigned && state.tunable.allowSignedIntegerLength1Bit) {
+ state.SDW(
+ WarnID.SignedBinaryIntegerLength1Bit,
+ outOfRangeFmtStr,
+ signedStr,
+ minWidth,
+ nBits
+ )
+ return false
+ } else {
+ UE(
+ state,
+ outOfRangeFmtStr,
+ signedStr,
+ minWidth,
+ nBits
+ )
+ return false
+ }
+ }
+ }
+ if (primNumeric.maxWidth.isDefined) {
+ val width = primNumeric.maxWidth.get
+ if (nBits > width) {
+ UE(
+ state,
+ "Number of bits %d out of range, must be between 1 and %d bits.",
+ nBits,
+ width
+ )
+ return false
Review Comment:
Same here, `return false` can be removed.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/BinaryNumberParsers.scala:
##########
@@ -166,9 +163,36 @@ abstract class BinaryIntegerBaseParser(
def parse(start: PState): Unit = {
val nBits = getBitLength(start)
- if (nBits == 0) return // zero length is used for outputValueCalc often.
- if (primNumeric.width.isDefined) {
- val width = primNumeric.width.get
+ if (primNumeric.minWidth.isDefined) {
+ val isSigned = primNumeric.isSigned
+ val signedStr = if (isSigned) "signed" else "unsigned"
Review Comment:
Move isSigned/signedStr to the inner block where it's used.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala:
##########
@@ -70,11 +71,16 @@ abstract class PackedBinaryDecimalBaseParser(
abstract class PackedBinaryIntegerBaseParser(
override val context: ElementRuntimeData,
- signed: Boolean = false
) extends PrimParser
with PackedBinaryConversion {
override lazy val runtimeDependencies = Vector()
+ val signed = {
+ context.optPrimType.get match {
+ case n: NodeInfo.PrimType.PrimNumeric => n.isSigned
+ case _ => false
Review Comment:
Also, this makes me concern the sign change wasn't correct? Previously we
passed in `false` to a handful of these packed parsers, regardless of the type.
I wonder if we don't allow unsigned primitives for those packed parsers, so
using primType.isSigned is functionally the same?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala:
##########
@@ -70,11 +71,16 @@ abstract class PackedBinaryDecimalBaseParser(
abstract class PackedBinaryIntegerBaseParser(
override val context: ElementRuntimeData,
- signed: Boolean = false
) extends PrimParser
with PackedBinaryConversion {
override lazy val runtimeDependencies = Vector()
+ val signed = {
+ context.optPrimType.get match {
+ case n: NodeInfo.PrimType.PrimNumeric => n.isSigned
+ case _ => false
Review Comment:
I'm surprised the default case has coverage. This doesn't seem possible to
me. A packed binary number should always have a numeric type. I wonder if
codecov just can't detect this correctly?
--
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]
