stevedlawrence commented on code in PR #1567:
URL: https://github.com/apache/daffodil/pull/1567#discussion_r2392374985
##########
daffodil-core/src/main/scala/org/apache/daffodil/unparsers/runtime1/BCDUnparsers.scala:
##########
@@ -69,41 +72,59 @@ final class BCDIntegerMinimumLengthUnparser(e:
ElementRuntimeData)
}
}
-abstract class BCDDecimalBaseUnparser(e: ElementRuntimeData,
binaryDecimalVirtualPoint: Int)
- extends PackedBinaryDecimalBaseUnparser(e, binaryDecimalVirtualPoint) {
-
- override def fromBigInteger(bigInt: JBigInteger, nBits: Int): Array[Byte] =
+abstract class BCDDecimalBaseUnparser(
+ e: ElementRuntimeData,
+ binaryDecimalVirtualPoint: Int,
+ decimalSigned: YesNo
+) extends PackedBinaryDecimalBaseUnparser(e, binaryDecimalVirtualPoint,
decimalSigned) {
+
+ override def fromBigInteger(bigInt: JBigInteger, nBits: Int): Array[Byte] = {
+ if (bigInt.signum == -1) {
+ UnparseError(
+ One(e.schemaFileLocation),
+ Nope,
+ "Signed numbers with dfdl:binaryNumberRep 'bcd' are always only
positive. %s cannot be negative",
Review Comment:
If BCD cannot have negative numbers should we even bother passing in/using
decimalSigned to all the BCD parsers/unparsers? And if the base class needs it,
can we instead just pass in `YesNo.No` to `PackedBinaryDecimalBaseUnparser` and
rely on the check in the base class to error if the number is negative?
I guess a downside is it's potentially confusing if the error message says
"number is negative but decimalSigned is no" even if the actual decimalSigned
property is "yes". But we can argue/document that the value of
`dfdl:decimalSigned` is ignored and implied to be "no" when `type="xs:decimal"`
and `dfdl:binaryNumberRep="bcd"`, so maybe it's still a reasonable diagnostic?
Note that we do this implied value of properties thing in a few places. For
example `dfdl:representation` is implied to be `text` when the type is
`xs:string`, so that feels consistent with the DFDL spec.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala:
##########
@@ -111,6 +113,10 @@ abstract class PackedBinaryDecimalBaseParser(
try {
val dec = toPrimType(context, dis.getByteArray(nBits, start))
+ if (dec.getBigDecimal.signum() == -1 && decimalSigned == YesNo.No) {
+ PE(start, "Packed binary decimal value is negative, but decimalSigned
is no.")
Review Comment:
Should we make the error message say `dfdl:decimalSigned` to make it more
clear the issue is with a DFDL property.
Also, is there any value in include the actual number in the diagnostics?
That might make debugging easier to show what a data parsed to. I'm not sure
what our convention is for including parsed values in diagnostics. If we do
that in other places, it might be worth adding it here so we're consistent.
Same with the other new errors.
##########
daffodil-core/src/main/scala/org/apache/daffodil/unparsers/runtime1/PackedBinaryUnparserTraits.scala:
##########
@@ -100,6 +102,14 @@ abstract class PackedBinaryDecimalBaseUnparser(
binaryDecimalVirtualPoint
)
}
+ if (bigDec.signum() == -1 && decimalSigned == YesNo.No) {
+ UnparseError(
+ One(e.schemaFileLocation),
+ Nope,
+ "Packed binary decimal value is negative, but decimalSigned is no",
+ context.dpathElementCompileInfo.namedQName.toPrettyString
Review Comment:
The format string doesn't have a `%s` in it, so I don't think this argument
is going to be used. I think that's fine, since I *think* the namedQName should
already be in the diagnostic context anyways? So I would suggest just removing
the argument.
--
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]
