tuxji commented on code in PR #918:
URL: https://github.com/apache/daffodil/pull/918#discussion_r1080712207
##########
daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/ex_nums.dfdl.xsd:
##########
@@ -97,14 +97,14 @@
<xs:element name="be_int32" type="xs:int"/>
<xs:element name="be_int64" type="xs:long"/>
<xs:element name="be_int8" type="xs:byte"/>
- <xs:element name="be_integer17" type="xs:integer"
+ <xs:element name="be_int17" type="xs:int"
dfdl:length="17"
dfdl:lengthKind="explicit"/>
<xs:element name="be_uint16"
type="xs:unsignedShort"/>
<xs:element name="be_uint32"
type="xs:unsignedInt"/>
<xs:element name="be_uint64"
type="xs:unsignedLong"/>
<xs:element name="be_uint8"
type="xs:unsignedByte"/>
- <xs:element name="be_nonNegativeInteger31"
type="xs:nonNegativeInteger"
+ <xs:element name="be_uint31" type="xs:unsignedInt"
dfdl:length="31"
Review Comment:
A lot of users' schemas potentially may need changes like this. We really
should be certain that lengthUnits="bits" cannot be used with integer and
nonNegativeInteger as I discussed above (another interpretation is that we
could keep type="nonNegativeInteger" and lengthUnits="bits" if we change
length="31" to length="32" and warn only if length is not a multiple of 8 bits
which might reduce the impact slightly).
##########
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -103,6 +103,14 @@ trait ElementBaseGrammarMixin
lazy val prefixedLengthElementDecl: PrefixLengthQuasiElementDecl =
LV('prefixedLengthElementDecl){
Assert.invariant(lengthKind == LengthKind.Prefixed)
+ optPrimType match {
+ case Some(primType) => primType match {
+ case PrimType.Integer | PrimType.NonNegativeInteger if lengthUnits ==
LengthUnits.Bits =>
+ SDW(WarnID.DeprecatedTypeUse, "In a future release,
lengthUnits='bits' will only be supported for integer types of 64 bits or less")
Review Comment:
If the change really must be made this way (disallowing lengthUnits="bits"
instead of checking that length is a multiple of 8 bits), then this error
message should say:
`In a future release, lengthUnits='bits' will not be supported for
xs:integer and xs:nonNegativeInteger types`
Note that we possibly might need code to disallow using lengthUnits="bits"
with xs:decimal types too.
##########
daffodil-test/src/test/resources/org/apache/daffodil/section12/lengthKind/ExplicitTests.tdml:
##########
@@ -903,4 +903,37 @@
</tdml:infoset>
</tdml:parserTestCase>
+<!--
+ Test Name: invalidLengthUnits_explicit
+ Schema: invalidLengthUnits_explicit
+ Root: r1
+ Purpose: This test checks that a warning is emitted when a bigint is
used when lengthUnits='bits'.
+-->
+ <tdml:defineSchema name="invalidLengthUnits_explicit">
+ <xs:include
schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:defineFormat name="Binary">
+ <dfdl:format ref="ex:GeneralFormat" representation="binary"/>
+ </dfdl:defineFormat>
+ <dfdl:format ref="ex:Binary" />
+
+ <xs:element name="r1" dfdl:lengthKind="explicit" type="xs:integer"
dfdl:lengthUnits="bits" dfdl:length="8"/>
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="invalidLengthUnits_explicit" root="r1"
model="invalidLengthUnits_explicit"
+ description="">
+ <tdml:document>
+ <tdml:documentPart type="byte">
+ 0f
+ </tdml:documentPart>
+ </tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>f
+ <r1>15</r1>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:warnings>
+ <tdml:warning>lengthUnits='bits' will only be supported for integer
types of 64 bits or less</tdml:warning>
Review Comment:
Also, best practice in `<tdml:warning>` is to look only for two or three
important words, so that the tests can be more robust to future error or
warning message changes.
##########
daffodil-test/src/test/resources/org/apache/daffodil/section05/simple_types/SimpleTypesUnparse.tdml:
##########
@@ -118,7 +118,7 @@
<xs:element name="float_01" type="xs:float" />
<xs:element name="double_01" type="xs:double" />
- <xs:element name="integer_01" type="xs:integer"
+ <xs:element name="integer_01" type="xs:int"
dfdl:lengthKind="explicit" dfdl:lengthUnits="bits" dfdl:length="8" />
Review Comment:
This is an example of a test schema which wouldn't need any changes under
the interpretation that length must be a multiple of 8 bits.
##########
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd:
##########
@@ -662,6 +662,7 @@
<xs:enumeration value="deprecatedPropertyDFDLXError" />
<xs:enumeration value="deprecatedPropertyDFDLError" />
<xs:enumeration value="deprecatedPropertySeparatorPolicy" />
+ <xs:enumeration value="deprecatedTypeUse" />
Review Comment:
Please choose a better ID than `deprecatedTypeUse`, maybe
`deprecatedBigIntegerBits`.
##########
daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/ex_nums.dfdl.xsd:
##########
@@ -150,7 +150,7 @@
dfdl:byteOrder="littleEndian"/>
<xs:element name="le_uint8" type="xs:unsignedByte"
dfdl:byteOrder="littleEndian"/>
- <xs:element name="le_nonNegativeInteger10"
type="xs:nonNegativeInteger"
+ <xs:element name="le_uint10" type="xs:unsignedInt"
dfdl:byteOrder="littleEndian"
dfdl:length="10"
Review Comment:
I also still want ex_nums to test the integer and nonNegativeInteger types,
but we'll need new elements for them. You don't have to do it in this PR - I
can add the new elements later.
--
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]
