-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert BigInt/Decimal to expected types for packed numbers #1391
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,9 +34,8 @@ | |
) extends PackedBinaryDecimalBaseParser(e, binaryDecimalVirtualPoint) | ||
with HasKnownLengthInBits { | ||
|
||
override def toBigInteger(num: Array[Byte]): JBigInteger = DecimalUtils.bcdToBigInteger(num) | ||
override def toBigDecimal(num: Array[Byte], scale: Int): JBigDecimal = | ||
DecimalUtils.bcdToBigDecimal(num, scale) | ||
override def toNumber(num: Array[Byte]): JBigDecimal = | ||
DecimalUtils.bcdToBigDecimal(num, binaryDecimalVirtualPoint) | ||
|
||
} | ||
|
||
|
@@ -48,9 +47,8 @@ | |
) extends PackedBinaryDecimalBaseParser(e, binaryDecimalVirtualPoint) | ||
with HasRuntimeExplicitLength { | ||
|
||
override def toBigInteger(num: Array[Byte]): JBigInteger = DecimalUtils.bcdToBigInteger(num) | ||
override def toBigDecimal(num: Array[Byte], scale: Int): JBigDecimal = | ||
DecimalUtils.bcdToBigDecimal(num, scale) | ||
override def toNumber(num: Array[Byte]): JBigDecimal = | ||
DecimalUtils.bcdToBigDecimal(num, binaryDecimalVirtualPoint) | ||
Check warning on line 51 in daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/BCDParsers.scala Codecov / codecov/patchdaffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/BCDParsers.scala#L51
|
||
|
||
} | ||
|
||
|
@@ -64,9 +62,8 @@ | |
) extends PackedBinaryDecimalBaseParser(e, binaryDecimalVirtualPoint) | ||
with PrefixedLengthParserMixin { | ||
|
||
override def toBigInteger(num: Array[Byte]): JBigInteger = DecimalUtils.bcdToBigInteger(num) | ||
override def toBigDecimal(num: Array[Byte], scale: Int): JBigDecimal = | ||
DecimalUtils.bcdToBigDecimal(num, scale) | ||
override def toNumber(num: Array[Byte]): JBigDecimal = | ||
DecimalUtils.bcdToBigDecimal(num, binaryDecimalVirtualPoint) | ||
|
||
override def childProcessors: Vector[Processor] = Vector(prefixedLengthParser) | ||
|
||
|
@@ -83,19 +80,15 @@ | |
) extends PackedBinaryIntegerBaseParser(e) | ||
with HasRuntimeExplicitLength { | ||
|
||
override def toBigInteger(num: Array[Byte]): JBigInteger = DecimalUtils.bcdToBigInteger(num) | ||
override def toBigDecimal(num: Array[Byte], scale: Int): JBigDecimal = | ||
DecimalUtils.bcdToBigDecimal(num, scale) | ||
override def toNumber(num: Array[Byte]): JBigInteger = DecimalUtils.bcdToBigInteger(num) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here the return type is different. toNumber returns a JBigInteger. I'm surprised this even works. JBigInteger must be a sub-type of JBigDecimal. |
||
|
||
} | ||
|
||
class BCDIntegerKnownLengthParser(e: ElementRuntimeData, val lengthInBits: Int) | ||
extends PackedBinaryIntegerBaseParser(e) | ||
with HasKnownLengthInBits { | ||
|
||
override def toBigInteger(num: Array[Byte]): JBigInteger = DecimalUtils.bcdToBigInteger(num) | ||
override def toBigDecimal(num: Array[Byte], scale: Int): JBigDecimal = | ||
DecimalUtils.bcdToBigDecimal(num, scale) | ||
override def toNumber(num: Array[Byte]): JBigInteger = DecimalUtils.bcdToBigInteger(num) | ||
|
||
} | ||
|
||
|
@@ -108,9 +101,7 @@ | |
) extends PackedBinaryIntegerBaseParser(e) | ||
with PrefixedLengthParserMixin { | ||
|
||
override def toBigInteger(num: Array[Byte]): JBigInteger = DecimalUtils.bcdToBigInteger(num) | ||
override def toBigDecimal(num: Array[Byte], scale: Int): JBigDecimal = | ||
DecimalUtils.bcdToBigDecimal(num, scale) | ||
override def toNumber(num: Array[Byte]): JBigInteger = DecimalUtils.bcdToBigInteger(num) | ||
|
||
override def childProcessors: Vector[Processor] = Vector(prefixedLengthParser) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this to toNumber? The return type is not JNumber.
Is there scaladoc on the toNumber method? There needs to be to explain this naming convention. I don't really understand why this got renamed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trait these classes mix in requires an implementation of
def toNumber(num: Array[Byte]): A
, whereA <: Number
. We could remove theA
type parameter and just make it so it always returns aJNumber
, but then implementations have to be a bit more careful to return the right thing.As it is right now, the type parameter ensures the implementations of
toNumber
return the right kind ofJNumber
(some should always return a BigInteger, some should always return a BigDecimal). I'm not sure the type parameter is used anywhere else though, so maybe that's overkill and just adds unnecessary complication?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the type parameter is almost certainly the right thing, I just don't see a change that adds this toNumber super method declaration, yet it's not in the current Daffodil main branch, so when did this type parameter get added?
But what is the purpose of the toNumber method? I think it is "convert to a number type we can store in the Infoset object. This should be the most specific meaningful subtype of JNumber" that is used in the Infoset object representation. Or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The top level declaration of toNumber is in PackedBinaryTraits.scala where it is simply declared as
The actual implementations are in the classes that extend the PackedBinaryConversion trait, like BCDIntegerKnownLengthParser.
The function will only generate either BigInteger's or BigDecimal's depending on the calling classes type, so perhaps a better name for the function would be "toBigNumber". The BigNumber then gets converted to the appropriate PrimType by calling PrimNumberic.fromNumber(bigNumber). This call is made by the toPrimType function of the same trait.