tuxji commented on code in PR #918:
URL: https://github.com/apache/daffodil/pull/918#discussion_r1081687819
##########
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -103,6 +103,7 @@ trait ElementBaseGrammarMixin
lazy val prefixedLengthElementDecl: PrefixLengthQuasiElementDecl =
LV('prefixedLengthElementDecl){
Assert.invariant(lengthKind == LengthKind.Prefixed)
+ assertLengthUnits()
Review Comment:
Note that `assertLengthUnits` calls SDE/SDW rather than an Assert method.
Consider renaming to something like `checkLengthUnits`.
##########
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -546,7 +547,9 @@ trait ElementBaseGrammarMixin
private def explicitBinaryLengthInBits() = {
val lengthFromProp: JLong = repElement.lengthEv.optConstant.get
val nbits = repElement.lengthUnits match {
- case LengthUnits.Bits => lengthFromProp.longValue()
+ case LengthUnits.Bits =>
+ assertLengthUnits(repElement)
+ lengthFromProp.longValue()
Review Comment:
I notice that this call passes `repElement` to assertLengthUnits while the
previous call passes the default argument `context`. More to say below.
##########
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd:
##########
@@ -101,6 +101,17 @@
<xs:element name="tunables">
<xs:complexType>
<xs:all>
+ <xs:element name="allowBigIntegerBits" type="xs:boolean"
default="true" minOccurs="0">
+ <xs:annotation>
+ <xs:documentation>
+ Daffodil allows the use of lengthUnits="bits" when the type is
xs:integer and
+ xs:nonNegativeInteger even though this is prohibited by the
specification. When
+ this tunable is true, a deprecation warning is emitted when this
condition is
+ encountered. When this tunable is false, this condition will
instead result in a
+ schema definition error.
Review Comment:
We've changed the check from disallowing 2 indefinite size integer types to
allowing only 9 definite size integer types which means that users will get
warnings or errors for many more types (float, double, etc.). I think it's
important to reword this documentation so that users will know how much this
tunable will impact their schemas. Remember Daffodil used to let many schema
writers pick one length unit (bits or bytes) and write every type's length
using that unit without any schema diagnostics, so the new SDE/SDW diagnostic
will be a big change in behavior for users. Consider saying something like:
```
Previous Daffodil releases let schemas define every type's length using
"bits" as the length unit
even though the specification allows bit length units only for a specific
set of types' binary
representations and does not allow bit length units for any other type's
binary representation
or any type's text representation. When this tunable is true, a deprecation
warning is issued
when bit length units are incorrectly used. When this tunable is false, a
schema definition
error will be issued instead.
```
##########
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1368,4 +1371,32 @@ trait ElementBaseGrammarMixin
mtaBase
}
+ val allowedBitTypes = Set[PrimType](
+ PrimType.Boolean,
+ PrimType.Byte,
+ PrimType.Short,
+ PrimType.Int,
+ PrimType.Long,
+ PrimType.UnsignedByte,
+ PrimType.UnsignedShort,
+ PrimType.UnsignedInt,
+ PrimType.UnsignedLong,
+ )
+ val allowedBitTypesText = allowedBitTypes.map("xs:" +
_.toString).toList.sorted.mkString(", ")
+
+ private def assertLengthUnits(elem: ElementBase = context): Unit = {
+ elem.lengthUnits match {
+ case LengthUnits.Bits if elem.representation == Representation.Binary =>
+ elem.optPrimType match {
+ case Some(primType) =>
+ if (!allowedBitTypes.contains(primType))
+ if (tunable.allowBigIntegerBits)
+ SDW(WarnID.DeprecatedBigIntegerBits, s"In a future release,
lengthUnits='bits' will only be supported for the following types:
$allowedBitTypesText")
+ else
+ SDE("lengthUnits='bits' is only supported for the following
types: $allowedBitTypesText")
Review Comment:
Since `elem` can be `context` or `repElement`, I wonder if we need to call
SDE/SDW on `elem` instead of `this` since the SDE/SDW methods include the
calling object's source location in the printed message?
--
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]
