stevedlawrence commented on code in PR #1337:
URL: https://github.com/apache/daffodil/pull/1337#discussion_r1840380013
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala:
##########
@@ -51,7 +52,15 @@ abstract class PackedBinaryDecimalBaseParser(
def parse(start: PState): Unit = {
val nBits = getBitLength(start)
- if (nBits == 0) return // zero length is used for outputValueCalc often.
+ if (nBits == 0) {
+ PE(
+ start,
+ "Number of bits %d out of range for a binary decimal. " +
+ "An unsigned decimal with length 1 bit could be used instead.",
Review Comment:
I'm not sure this error message is correct. I believe packed numbers have
different rules about minimum lengths. It's not the same as 2's complement.
Also, this message references "binary decimal", which I think implies 2's
complement. Any error messages here should probably reference packed decimals,
or leave it up to individual packed parsers to detect bitLength errors and give
specific errors.
I think what was intended with the packed implementation is we just call
`toBigDecimal`/`toBigInteger` below and the different packed implementations do
their parsing/validation and throw an exception if it's not a valid
length/value, and that exception is converted into a PE.
So I think all this 1-bit signed stuff done elsewhere doesn't even apply to
packed decimals. Packed decimals should already be doing bit length checking
specific to the packed format. If they handle the zero case correctly, we could
probably even remove this check/PE. If not, we probably still need it, but
should have an error specific to zero length packed decimals not allowed.
It might also be worth adding a test for some zero length packed things so
we get coverage of this edge case.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -268,6 +306,32 @@ abstract class BinaryDecimalUnparserBase(
nBits: Int,
finfo: FormatInfo
): Boolean = {
+ val state = finfo.asInstanceOf[UState]
+ val isSigned = signed == Yes
+ val minWidth = if (isSigned) 2 else 1
+ if (nBits < minWidth) {
+ val signedStr = if (isSigned) "a signed" else "an unsigned"
+ val outOfRangeFmtStr =
+ "Minimum length for %s binary decimal is %d bit(s), number of bits %d
out of range. " +
+ "An unsigned decimal with length 1 bit could be used instead."
+ if (isSigned && state.tunable.allowSignedIntegerLength1Bit && nBits ==
1) {
+ state.SDW(
+ WarnID.SignedBinaryIntegerLength1Bit,
+ outOfRangeFmtStr,
+ signedStr,
+ minWidth,
+ nBits
+ )
+ } else {
+ UE(
+ state,
+ outOfRangeFmtStr,
+ signedStr,
+ minWidth,
+ nBits
+ )
Review Comment:
I think this same chunk of code with very minor variations appears in
multiple places. Can we add trait to avoid all this duplication?
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/BinaryNumberUnparsers.scala:
##########
@@ -78,17 +76,58 @@ 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 minWidth = primNumeric.minWidth.get
+ if (nBits < minWidth) {
+ val isSigned = primNumeric.isSigned
+ val signedStr = if (isSigned) "a signed" else "an unsigned"
+ val outOfRangeFmtStr =
+ "Minimum length for %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 && nBits ==
1) {
+ state.SDW(
Review Comment:
Are there any concerns about creating SDW's at parse time? Diagnostic
overhead should be small, but imagine an array of a bunch of 1-bit length
elements--that could create a lot of diagnostics. Also consider that this same
warning was already output during compilation unless nbits was calculated at
runtime. Maybe the compile time warning is sufficient and we should just be
silent at runtime? Or maybe we need a flag so we only warn here only if we're
in the `BinaryIntegerKnownLengthUnparser`?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala:
##########
@@ -69,17 +78,35 @@ abstract class PackedBinaryDecimalBaseParser(
}
abstract class PackedBinaryIntegerBaseParser(
- override val context: ElementRuntimeData,
- signed: Boolean = false
+ override val context: ElementRuntimeData
) extends PrimParser
with PackedBinaryConversion {
override lazy val runtimeDependencies = Vector()
+ val signed = {
+ context.optPrimType.get match {
+ case n: NodeInfo.PrimType.PrimNumeric => n.isSigned
+ // context.optPrimType can be of type date/time via
ConvertZonedCombinator
+ case _ => false
+ }
+ }
protected def getBitLength(s: ParseOrUnparseState): Int
def parse(start: PState): Unit = {
val nBits = getBitLength(start)
- if (nBits == 0) return // zero length is used for outputValueCalc often.
+ if (nBits == 0) {
+ val signedStr = if (signed) "a signed" else "an unsigned"
+ val minWidth = if (signed) 2 else 1
+ PE(
+ start,
+ "Minimum length for %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.",
+ signedStr,
+ minWidth,
+ nBits
+ )
+ return
+ }
Review Comment:
Same question as above, I'm not sure this error makes sense for packed
numbers and can possibly be removed?
--
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]
