mbeckerle commented on code in PR #1337:
URL: https://github.com/apache/daffodil/pull/1337#discussion_r1871920255
##########
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd:
##########
@@ -137,6 +137,14 @@
</xs:documentation>
</xs:annotation>
</xs:element>
+ <xs:element name="allowSignedIntegerLength1Bit" type="xs:boolean"
default="true" minOccurs="0">
Review Comment:
Note that this change and tunable requires a release note in the next
release suggesting that people update schemas to not depend on 1-bit signed
binary integers.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala:
##########
@@ -51,7 +52,14 @@ 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,
Review Comment:
This does need test coverage. It may be possible to remove this check
entirely. It may not be reachable in that something else is checking for the
zero case before this is called?
##########
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:
Yeah, I think you need to know if the decimal representation is expected to
be signed or not. The error case of dfdl:decimalSigned="no" but the packed
representation has a negative sign needs to be detected somehow.
One could choose different parse primitives based on dfdl:decimalSigned
yes/no or pass a boolean isSigned to a common primitive.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala:
##########
@@ -69,17 +77,30 @@ 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) {
Review Comment:
Somewhere else we must be checking for packed length not a multiple of 4
bits, and having room for the packed sign nibble, etc. So I suspect we don't
need to check for zero bits as a special case here.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedDecimalParsers.scala:
##########
@@ -84,11 +84,10 @@ class PackedDecimalPrefixedLengthParser(
class PackedIntegerRuntimeLengthParser(
val e: ElementRuntimeData,
- signed: Boolean,
packedSignCodes: PackedSignCodes,
val lengthEv: Evaluatable[JLong],
val lengthUnits: LengthUnits
-) extends PackedBinaryIntegerBaseParser(e, signed)
+) extends PackedBinaryIntegerBaseParser(e)
Review Comment:
I think you need to pass the signed boolean.
--
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]
