stevedlawrence commented on code in PR #1337:
URL: https://github.com/apache/daffodil/pull/1337#discussion_r1820739495
##########
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:
Hmm, seems like we have a bug in that parse and unparse are not symmetric.
For parse,
* 0x1 parses to 1
* 0x0 parses to 0
For unparse,
* 1 unparses to 0x0
* 0 unparse to 0x0
* -1 unparses to 0x1
Since parse will never create a -1 in the infoset, it means it will always
unparse to 0 unless someone modifies the infoset, which is unlikely.
Considering this, I wonder if we should remove the new tunable and just drop
support for signed 1-bit integers completely? If someone was already using this
and roundtripping data, things were going to be subtly buggy. So maybe
changing this to an error is going to make it more obvious that things are
broken and people can fix their schema?
And maybe we just add a deprecation/compatibility note that signed 1-bit
integers were broken and shouldn't be supported anyways, so they are 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]
