Don't require binaryNumberRep for hexBinary #155
Conversation
| private def getImplicitAlignmentInBits(thePrimType: PrimType, theRepresentation: Representation): Int = { | ||
| (theRepresentation, thePrimType) match { | ||
| case (Representation.Text, PrimType.HexBinary) => Assert.impossible("type xs:hexBinary with representation='text'") | ||
| case (Representation.Text, _) => knownEncodingAlignmentInBits | ||
| case (Representation.Binary, PrimType.Float | PrimType.Boolean) => 32 | ||
| case (Representation.Binary, PrimType.Double) => 64 | ||
| case (Representation.Binary, _) => binaryNumberRep match { | ||
| case (Representation.Binary, PrimType.HexBinary) => 8 | ||
| case (Representation.Binary, _) if (optionBinaryNumberRep.isDefined) => binaryNumberRep match { |
There was a problem hiding this comment.
Is the optionBinaryNumberRep.isDefine check needed here? If the prim type is hexBInary it'll hit the new case you added and will never try to get optionBInaryNumberRep, I think? There's also a case PrimType.HexBinary => 8 at the bottom of this match case that I think is impossible to hit with the above change? Hard to tell with this complex nesting of match cases...
There was a problem hiding this comment.
I think this rats nest of match cases was made by me, so I took a stab at greatly simplifying the logic here and I think it is much cleaner now. I ran the test suite with these changes and everything passes, but give it a look to make sure I didn't miss anything obvious.
| @@ -701,6 +704,7 @@ trait ElementBase | |||
| alignInBits, implicitAlignmentInBits, primType.name, this.knownEncodingName) | |||
| } | |||
| case Representation.Binary => primType match { | |||
| case PrimType.HexBinary => 8 | |||
There was a problem hiding this comment.
I think the 8 here does nothing? This part of this val is just doing validation like making sure packed decimals are aligned. We don't have that restriction with hex binary since we allow hexbinary to be non-byte sized and non-byte aligned. So I think this can just be a no-op like the Float/Double/Booleans below it?
| @@ -653,29 +653,29 @@ trait ElementBase | |||
| } | |||
| } | |||
|
|
|||
| private lazy val optionBinaryNubmerRep = findPropertyOption("binaryNumberRep") | |||
| /* All binary number representations other than "binary" are of some packed format */ | |||
| private lazy val isPackedBinaryNumber: Boolean = optionBinaryNumberRep.isDefined && (binaryNumberRep != BinaryNumberRep.Binary) | |||
There was a problem hiding this comment.
I'm not sure if this is right. This says that if the binaryNumberRep property is not defined then the number can't be packed. But the spec is kindof the opposite of fhat. It says that if a primtype is one of certain types (e.g. long, integer, not hexbary, not float), then the binaryNumberRep must be defined, otherwise it doesn't need to be defined. So we should never base logic on whether ornot binaryNumberRep isdefined, but shoulde ither demend it exists or doesn't based on the prim Type. I like the cleanup 👍, but I dont think the logic is completely correct.
Also, this is just stylistic, but I kindof find the pipes in case hard to read in these bigmatch cases. It makes this compact, but I''m not sure it enhances readability. While you're reactory, it would be good to get rid of the pipes in this function.
There was a problem hiding this comment.
I have updated the code to not rely on whether or not binaryNumberRep is defined.
I have also tried to tidy up the areas with all the pipes, but I still prefer the use of pipes instead of having several more case statements that evaluate to the same thing.
The property binaryNumberRep should not need to be defined for hexBinary elements. Also cleaned up logic for complicated match statement when dealing with alignment for binary numbers. DAFFODIL-2031
jadams-tresys
force-pushed
the
daffodil-2031-hexBinary-binaryNumberRep
branch
from
327ee44
to
5d0a11c
Compare
The property binaryNumberRep should not need to be defined for hexBinary
elements.
DAFFODIL-2031