stevedlawrence commented on code in PR #1337:
URL: https://github.com/apache/daffodil/pull/1337#discussion_r1799857190
##########
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 would suggest we come up with a single tunable that covers both cases
(zero length numbers and 1-bit length signed integers), since this tunable
really just wants to be a way to slowly deprecate the current/bad behavior. For
now we probably want to default that tunable to the curent/bad behavior, but
eventually we'll want to change the default use the correct behavior, while
still allowing the old behavior if schemas really do depend on it.
##########
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:
> the issue is putNumber doesn't have access to state
When we call putNumber we actually do pass in the state, it is just upcast
to a `FormatInfo` so `putNumber` doesn't realize it's actually a UState. But we
can change the `FormatInfo` parameter to `UState` and it should all just work.
I don't think there's a reason putNumber must take a `FormatInfo` instead of a
`UState`.
> BinaryNumberBaseUnparser is used by non Binary Integer classes
Could we put all the width logic in `BinaryIntegerBaseUnparser.putNumber()`.
Then it will only apply to integer types, which I think is exactly what we
want?
##########
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 think you can have `representation="binary"` with `lengthKind="pattern"`.
That might trigger the code path? Note that you probably don't even need the
regex lookahead to trigger this. In a bits encoding, `dfdl:lengthPattern="."`
should match just a single bit.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -774,10 +774,32 @@ trait ElementBase
if (
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 nBits = result.get
val width = primNumeric.width.get
Review Comment:
Thoughts on renaming `width` to `maxWidth` and adding a new `minWidth`
member in NodeInfo. Avoids some of the special logic around figuring out if
stuff is signed or not. And then checking maxWidth and minWidth is very similar
logic, just something like
```scala
if (primNumeric.minWdith.isDefined) {
val minWidth = primNumeric.minWidth.get
if(nBits < minWidth) {
SDE(...)
}
}
```
And then the minWidth can be reused in the parser/unparser logic.
Maybe we also add a `signed` member so we don't have to have the
isSigned/isUnsigned stuff? And then parser/unparsers, which should already have
access to the primType can just reference the signed member? We pass around
isSigned in a bunch of places, wondering if maybe that can all be simplified.
--
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]
