mbeckerle commented on code in PR #800:
URL: https://github.com/apache/daffodil/pull/800#discussion_r891577703
##########
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.Daffodil,
validateTDMLFile: Boolean = true,
validateDFDLSchemas: Boolean = true,
compileAllTopLevel: Boolean = false,
defaultRoundTripDefault: RoundTrip = defaultRoundTripDefaultDefault,
defaultValidationDefault: String = defaultValidationDefaultDefault,
defaultImplementationsDefault: Seq[String] =
defaultImplementationsDefaultDefault): Runner = {
- val resourceDir = if (dir.startsWith("/")) dir else "/" + dir
- val resourcePath = if (resourceDir.endsWith("/")) resourceDir + file else
resourceDir + "/" + file
+ // Prepend forward slash to turn dir/file into classpath resource
Review Comment:
We have Misc.getResourceOption and Misc.getRequiredResource which handle
this tedium and return you an Option[URI] or a URI which can then be opened.
via URI.openStream. So you can simplify this a bit. The diagnostic message I
think shows everywhere the resource was searched which helps with misconfigured
class path.
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -194,7 +198,7 @@ class DFDLTestSuite private[tdml] (
defaultImplementationsDefault: Seq[String] =
Runner.defaultImplementationsDefaultDefault,
shouldDoErrorComparisonOnCrossTests: Boolean =
Runner.defaultShouldDoErrorComparisonOnCrossTests,
shouldDoWarningComparisonOnCrossTests: Boolean =
Runner.defaultShouldDoWarningComparisonOnCrossTests) =
- this(null, aNodeFileOrURL, validateTDMLFile, validateDFDLSchemas,
compileAllTopLevel,
+ this(null, aNodeFileOrURL, useTDMLImplementation, validateTDMLFile,
validateDFDLSchemas, compileAllTopLevel,
Review Comment:
No coverage == no tests for this so maybe we can remove this deprecated
constructor now?
##########
daffodil-test/src/test/resources/org/apache/daffodil/runtime2/MPU_orange_to_green_60006.tdml:
##########
@@ -17,26 +17,19 @@
-->
<tdml:testSuite
- defaultConfig="config-runtime2"
- defaultImplementations="daffodil daffodil-runtime2"
defaultRoundTrip="onePass"
defaultValidation="on"
description="TDML tests for MPU_orange_to_green_60006.dfdl.xsd"
xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/";
xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData";>
- <tdml:defineConfig name="config-runtime1">
- <daf:tunables>
- <daf:tdmlImplementation>daffodil</daf:tdmlImplementation>
- </daf:tunables>
- </tdml:defineConfig>
-
- <tdml:defineConfig name="config-runtime2">
- <daf:tunables>
- <daf:tdmlImplementation>daffodil-runtime2</daf:tdmlImplementation>
- </daf:tunables>
- </tdml:defineConfig>
+ <!--
+ Run all tests:
Review Comment:
How about from "Run all tests:" to "To run all tests from the CLI:"
##########
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:
Probably just a bug in runtime 1. Getting this right with unparser
streaming, partial bytes, etc. is tricky.
If there is just one global fill byte, then fillByte shoujld work as if you
wrote the entire data stream full of fill byte on byte boundaries, and then
unparsed on top of that. That's the basic model. Except of course that the
fillByte, like any property, can vary in different schema components.
Given that there can be multiple elements within a byte, even text
characters can be less than a byte wide, it's unclear which component's fill
byte fills in the extra bits.
Suppose you have a single-bit-wide element with trailingSkip='1' and
alignmentUnits='bits'. Then that element's fillByte should provide the missing
bit, but, how to know what position that bit is at? You cannot know that until
you know the alignment of the element, which may be undetermined due to
variable-sized things preceding it in the unparse, the size of which cannot as
yet be determined.
So the alignmentFill region here for unparse gets suspended until the
starting bitPos0b is known.
So in runtime 1 unparsing, getting this right is in fact quite tricky.
And a low prio to fix because I've really only ever seen 0x00, 0xFF, and
0x20 as fill bytes. There are arguments that other things like
dfdl:fillByte='^' make looking at hex dumps easier to spot filled data bugs
(e.g., element length problems). It's a debug technique. Doesn't help for bits
within a byte though. That's not going to look like the "^" char.
##########
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:
I almost never use daffodil CLI to run tests.
I always run them by just letting junit run them. So I want the tests to
configure themselves for which runtime/backend within the TDML file itself, via
a config/tunable.
I see below that in the scala junit test driver code, you 've added a
parameter to specify which implementation for a test. The test itself specifies
which implementations it is compatible with.
This is a stable design point. Seems ok to me. Just makes it harder for me
to focus only on runtime 1 first as I am developing. But I can cope.
--
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]
