stevedlawrence commented on code in PR #800:
URL: https://github.com/apache/daffodil/pull/800#discussion_r890291203
##########
daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala:
##########
@@ -397,6 +407,10 @@ class CLIConf(arguments: Array[String]) extends
scallop.ScallopConf(arguments) {
descr("List or execute TDML tests")
helpWidth(width)
+ val implementation = opt[TDMLImplementation](short = 'I', argName =
"implementation",
+ descr = "Implementation to run TDML tests. Choose one of %s. Defaults to
%s."
+ .format(TDMLImplementation.allValues.mkString(", "),
TDMLImplementation.allValues.head.toString),
Review Comment:
Thoughts on using `TDMLImplementation.Daffodil` as the default? That way if
the order ever changes and thus `allValues.head` changes, we'll still default
to runtime1?
##########
daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala:
##########
@@ -397,6 +407,10 @@ class CLIConf(arguments: Array[String]) extends
scallop.ScallopConf(arguments) {
descr("List or execute TDML tests")
helpWidth(width)
+ val implementation = opt[TDMLImplementation](short = 'I', argName =
"implementation",
+ descr = "Implementation to run TDML tests. Choose one of %s. Defaults to
%s."
+ .format(TDMLImplementation.allValues.mkString(", "),
TDMLImplementation.allValues.head.toString),
Review Comment:
Another thing to consider, the "ibm" implementation probably won't work with
the Daffodil CLI by default. IBM needs a handful of classpath changes in order
for this to work correctly. I'm not sure it's worth worrying about, it might be
nice to allow it and then someone could eventually figure out exactly how the
classpath needs to changed. But just a consideration.
##########
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/parsers.c:
##########
@@ -507,26 +507,25 @@ parse_le_uint8(uint8_t *number, size_t num_bits, PState
*pstate)
*number = (uint8_t)integer;
}
-// Skip fill bytes until end bitPos0b is reached
+// Parse fill bits until end bitPos0b is reached
void
-parse_fill_bytes(size_t end_bitPos0b, PState *pstate)
+parse_fill_bits(size_t end_bitPos0b, PState *pstate)
{
Review Comment:
In the future, I'd prefer we split up unrelated changes into separate PRs. I
don't think the TDML implementation and the fill bits are really that related,
makes code review easier when everything is about one thing.
##########
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/unparsers.c:
##########
@@ -401,27 +401,24 @@ unparse_le_uint8(uint8_t number, size_t num_bits, UState
*ustate)
unparse_endian_uint64(LITTLE_ENDIAN_DATA, number, num_bits, ustate);
}
-// Add fill bytes until end bitPos0b is reached
+// Unparse fill bits until end bitPos0b is reached
void
-unparse_fill_bytes(size_t end_bitPos0b, const uint8_t fill_byte, UState
*ustate)
+unparse_fill_bits(size_t end_bitPos0b, const uint8_t fill_byte, UState *ustate)
{
- union
- {
- uint8_t bytes[1];
- } buffer;
- buffer.bytes[0] = fill_byte;
+ assert(ustate->bitPos0b <= end_bitPos0b);
- // Update our last successful write position only if this loop
- // finishes without any errors
- size_t current_bitPos0b = ustate->bitPos0b;
- while (current_bitPos0b < end_bitPos0b)
+ size_t fill_bits = end_bitPos0b - ustate->bitPos0b;
+ while (fill_bits)
{
- write_bits(buffer.bytes, BYTE_WIDTH, ustate);
+ size_t num_bits = (fill_bits >= BYTE_WIDTH) ? BYTE_WIDTH : fill_bits;
+ write_bits(&fill_byte, num_bits, ustate);
Review Comment:
Does this do fill bits correctly? The spec says
> If bit K ... of a data stream byte is unspecified, its value is taken from
bit K of the dfdl:fillByte property value.
So say we have already unparsed a single `0` bit and we need to fill the
remaining 7 bits to fill out the byte. Also assume the the fill byte is `0xF0.`
I *think* the way you have it, it will unparse to `0111 1000`, but it should
unparse to `0111 0000`, right?
That said, I think runtime1 doesn't implement this correctly either. In
fact, I think there's a serious bug? Say we have this schema that describes the
above scenario:
```xml
<schema
xmlns="http://www.w3.org/2001/XMLSchema";
xmlns:xs="http://www.w3.org/2001/XMLSchema";
xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/";>
<include
schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd" />
<annotation>
<appinfo source="http://www.ogf.org/dfdl/";>
<dfdl:format xmlns="" ref="GeneralFormat"
representation="binary"
lengthKind="explicit"
lengthUnits="bits"
fillByte="%#xF0;" />
</appinfo>
</annotation>
<element name="container" dfdl:length="8">
<complexType>
<sequence>
<element name="bit" type="xs:int" dfdl:length="1" />
</sequence>
</complexType>
</element>
</schema>
```
And this infoset to unparse:
```xml
<container>
<bit>0</bit>
</container>
```
Runtime1 currently unparses to `0011 1111`. I have no idea how it's getting
that result, but it seems very wrong to me. Maybe I'm just not understanding
fill byte correctly?
##########
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd:
##########
@@ -587,15 +587,23 @@
<xs:restriction base="dfdlx:ParseUnparsePolicyEnum" />
</xs:simpleType>
<xs:simpleType>
- <xs:restriction base="xs:string">
+ <xs:restriction base="xs:token">
<xs:enumeration value="fromRoot" />
</xs:restriction>
</xs:simpleType>
</xs:union>
</xs:simpleType>
+ <xs:simpleType name="TunableTDMLImplementation">
Review Comment:
Do we want to call this something other than "Tunable...". TDML
Implemementation isn't really a tunable anymore.
##########
daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala:
##########
@@ -397,6 +407,10 @@ class CLIConf(arguments: Array[String]) extends
scallop.ScallopConf(arguments) {
descr("List or execute TDML tests")
helpWidth(width)
+ val implementation = opt[TDMLImplementation](short = 'I', argName =
"implementation",
+ descr = "Implementation to run TDML tests. Choose one of %s. Defaults to
%s."
+ .format(TDMLImplementation.allValues.mkString(", "),
TDMLImplementation.allValues.head.toString),
+ default = TDMLImplementation.optionStringToEnum("I",
sys.env.getOrElse("TDML_IMPLEMENTATION", "")))
Review Comment:
I'm not sure I understand this. If TDML_IMPLEMENATION is defined we set the
default to that, if we don't we set it to empty string? Should it be something
like this instead:
```scala
default = {
val implStr = sys.env.getOrElse("TDML_IMPLEMENTATION",
TDMLImplementation.Daffodil.toString)
TDMLImplementation.optionStringToEnum("I", implStr)
}
```
Also, what happens if TDML_IMPLEMENTATION isn't valid? There's not really a
default anymore so I'm not sure what would happen.
##########
daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/ex_nums.tdml:
##########
@@ -158,4 +187,78 @@
</tdml:validationErrors>
</tdml:parserTestCase>
+ <tdml:parserTestCase
+ config="config-runtime2"
+ model="ex_nums.dfdl.xsd"
+ name="runtime2_error_on"
+ validation="on">
+ <tdml:document>
+ <tdml:documentPart type="file">ex_nums.error.dat</tdml:documentPart>
+ </tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ex_nums.error.dat.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:validationErrors>
+ <tdml:error>value</tdml:error>
+ <tdml:error>boolean_false</tdml:error>
+ <tdml:error>does not match</tdml:error>
+ </tdml:validationErrors>
+ </tdml:parserTestCase>
+
+ <!--
+ In runtime2, unparse always writes non-fixed values with validation
+ diagnostics (doesn't even have a validation option)
+
+ c/daffodil unparse -o c/ex_nums.error.dat ex_nums.error.dat.xml
+ -->
+
+ <tdml:unparserTestCase
+ config="config-runtime2"
+ model="ex_nums.dfdl.xsd"
+ name="runtime2_error_unparse">
+ <tdml:infoset>
Review Comment:
Is it worth adding `implementation="daffodilC"` to some of these so it's
clear that they are testing runtime2 specific behavior?
##########
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd:
##########
@@ -502,11 +502,11 @@
</xs:documentation>
</xs:annotation>
</xs:element>
- <xs:element name="tdmlImplementation" type="xs:string"
default="daffodil" minOccurs="0">
+ <xs:element name="tdmlImplementation"
type="daf:TunableTDMLImplementation" default="daffodil" minOccurs="0">
Review Comment:
Should we remove this tunable? It's not backwards compatible, but I suspect
this doesn't have many users.
##########
daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/ex_nums.tdml:
##########
@@ -38,21 +38,25 @@
<tdml:defineConfig name="config-runtime2">
<daf:tunables>
- <daf:tdmlImplementation>daffodil-runtime2</daf:tdmlImplementation>
+ <daf:tdmlImplementation>daffodilC</daf:tdmlImplementation>
</daf:tunables>
</tdml:defineConfig>
Review Comment:
Should we remove/deprecate this way to select the implementation?
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/RunnerFactory.scala:
##########
@@ -37,28 +39,48 @@ import org.apache.daffodil.util.Misc
*/
object Runner {
def apply(dir: String, file: String,
+ useTDMLImplementation: TDMLImplementation =
TDMLImplementation.allValues.head,
Review Comment:
Mentioned previously, but I'd consider not using `allValues.head` in case
order ever changes. We probably want to be explicit about what the default is.
I also wonder if it want `useTDMLImplementation` to be an `Option` that
defaults to `None`? And then the TDMLRunner can check if it's defined, abd if
it's not check TDML_IMPEMENATION, and if that's not defined then default to
runtime1? That way you could do something like
```
TDML_IMPLEMENTATION="daffodilC" sbt test
```
That would run all sbt tests that don't explicitly specify an implementation
to use runtime2. I'm not sure `sbt test` would be all that helpful due to the
differences in functionality, but doing something like `sbt testOnly
some.subset.of.tests` might be useful.
--
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]
