stevedlawrence commented on a change in pull request #155: Don't require
binaryNumberRep for hexBinary
URL: https://github.com/apache/incubator-daffodil/pull/155#discussion_r241765411
##########
File path:
daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -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)
Review 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 :+1:, 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
