tuxji commented on a change in pull request #466:
URL: https://github.com/apache/incubator-daffodil/pull/466#discussion_r543635266
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/Runtime2DataProcessor.scala
##########
@@ -108,6 +108,8 @@ class Runtime2DataProcessor(executableFile: os.Path)
extends DFDL.DataProcessorB
val parseResult = new ParseResult(outfile, Failure(parseError))
parseResult.addDiagnostic(parseError)
parseResult
+ } finally {
+ os.remove.all(tempDir)
Review comment:
Nope, these two are the only new dependencies. I've added their
copyrights to daffodil-cli/bin.LICENSE and in directories-jvm's case, I looked
for a prominent label to add to daffodil-cli/bin.NOTICE but I couldn't find any
such label in directories-jvm's github repo
(<https://github.com/dirs-dev/directories-jvm>).
Both new deps are used by the C code generator in runtime2. OS-Lib has some
other alternatives, but its advantage is that it has very concise and Scala-ish
APIs for operating on files and running processes. We probably could benefit
from using OS-Lib throughout Daffodil, not just in runtime2, to simplify code
that operates on files or runs processes. We probably could expand Daffodil
Utils instead to cover the use cases I've been using OS-Lib for if we needed to
as well.
I found no alternatives except writing our own Daffodil specific
implementation to allow Daffodil to create its own cache directory in the
"right" place on Linux, Mac, and Windows when I looked for and found dev-dirs /
directories-jvm to do exactly that. The C code generator (as far as I know) is
the only place (so far) which needs to use a cache directory, although maybe
there are other places in Daffodil that might find a use for directories-jvm
(besides finding the right place for a Daffodil cache directory, it also can
find the right place for a Daffodil config file inside a user's home directory).
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/BinaryIntegerKnownLengthCodeGenerator.scala
##########
@@ -36,50 +38,47 @@ trait BinaryIntegerKnownLengthCodeGenerator {
val bo = e.byteOrderEv.constValue
bo
}
- // For the time being this is a very limited back end.
- // So there are numerous restrictions to enforce.
- e.schemaDefinitionUnless(lengthInBits <= 64, "Length must be 64 bits or
less, but was: %s", lengthInBits)
- if (lengthInBits == 64 && !g.signed)
- e.SDE("Integers of 64 bits length must be signed.")
// This insures we can use regular java.io library calls.
if (e.alignmentValueInBits.intValue() % 8 != 0)
e.SDE("Only alignment to 8-bit (1 byte) boundaries is supported.")
// The restrictions below are ones we want to eventually lift.
- if (lengthInBits != 32)
- e.SDE("Lengths other than 32 bits are not supported.")
-
if (byteOrder ne ByteOrder.BigEndian)
e.SDE("Only dfdl:byteOrder 'bigEndian' is supported.")
if (e.bitOrder ne BitOrder.MostSignificantBitFirst)
e.SDE("Only dfdl:bitOrder 'mostSignificantBitFirst' is supported.")
- if (!g.signed)
- e.SDE("Only signed integers are supported.")
-
- val initStatement = s" instance->$fieldName = 0xCDCDCDCD;"
+ // Start generating code snippets
+ val initialValue = lengthInBits match {
+ case 8 => "0xCD"
+ case 16 => "0xCDCD"
+ case 32 => "0xCDCDCDCD"
+ case 64 => "0xCDCDCDCDCDCDCDCD"
+ case _ => e.SDE("Lengths other than 8, 16, 32, or 64 bits are not
supported.")
Review comment:
Yes, what Mike said. I've added a comment that those are magic debug
values (unusual memory bit patterns) to mark fields that do not have integer
values written to their locations for any reason.
##########
File path: daffodil-runtime2/src/main/resources/examples/ex_int32.c
##########
@@ -99,42 +99,45 @@ static const ERD c2_ERD = {
(ERDUnparseSelf)&c2_unparseSelf, // unparseSelf
};
-static const c1 c1_compute_ERD_offsets;
+static const ex_int32 ex_int32_compute_ERD_offsets;
-static const ptrdiff_t c1_offsets[2] = {
- (char *)&c1_compute_ERD_offsets.e1 - (char *)&c1_compute_ERD_offsets,
- (char *)&c1_compute_ERD_offsets.c2 - (char *)&c1_compute_ERD_offsets};
+static const ptrdiff_t ex_int32_offsets[2] = {
+ (char *)&ex_int32_compute_ERD_offsets.e1 - (char
*)&ex_int32_compute_ERD_offsets,
+ (char *)&ex_int32_compute_ERD_offsets.c2 - (char
*)&ex_int32_compute_ERD_offsets};
-static const ERD *c1_childrenERDs[2] = {&e1_ERD, &c2_ERD};
+static const ERD *ex_int32_childrenERDs[2] = {&e1_ERD, &c2_ERD};
-static const ERD c1_ERD = {
+static const ERD ex_int32_ERD = {
{
- "ex", // namedQName.prefix
- "c1", // namedQName.local
+ NULL, // namedQName.prefix
+ "ex_int32", // namedQName.local
"http://example.com";, // namedQName.ns
},
COMPLEX, // typeCode
2, // numChildren
- c1_offsets, // offsets
- c1_childrenERDs, // childrenERDs
- (ERDInitSelf)&c1_initSelf, // initSelf
- (ERDParseSelf)&c1_parseSelf, // parseSelf
- (ERDUnparseSelf)&c1_unparseSelf, // unparseSelf
+ ex_int32_offsets, // offsets
+ ex_int32_childrenERDs, // childrenERDs
+ (ERDInitSelf)&ex_int32_initSelf, // initSelf
+ (ERDParseSelf)&ex_int32_parseSelf, // parseSelf
+ (ERDUnparseSelf)&ex_int32_unparseSelf, // unparseSelf
};
// Return a root element to be used for parsing or unparsing
InfosetBase *
rootElement()
{
- static c1 instance;
+ static ex_int32 instance;
InfosetBase *root = &instance._base;
- c1_ERD.initSelf(root);
+ ex_int32_ERD.initSelf(root);
return root;
}
// Methods to initialize, parse, and unparse infoset nodes
+static inline uint8_t be8toh(uint8_t be8b) { return be8b; }
Review comment:
Yes, as you realized and commented below, these functions are degenerate
1-byte cases of endian-sensitive integer transformation functions defined in
the GNU C library (https://linux.die.net/man/3/be16toh). This ex_int32 example
doesn't use them but the orion_command example does have some 8-bit integers.
The "h" stands for host order which could be little-endian on a little-endian
architecture or big-endian on a big-endian architecture (the functions know
which architecture they're compiled on). Added comment and also added
little-endian versions "htole8" and "le8toh".
##########
File path: daffodil-runtime2/src/main/resources/examples/ex_int32.c
##########
@@ -149,7 +152,7 @@ c2_parseSelf(c2 *instance, const PState *pstate)
const char *error_msg = NULL;
if (!error_msg)
{
- char buffer[4];
+ char buffer[sizeof(uint32_t)];
Review comment:
The endian-sensitive conversion functions take and return only unsigned
integers, so I used unsigned types as temporary variables and in the "sizeof()"
as well for consistency.
##########
File path: project/Rat.scala
##########
@@ -114,7 +114,8 @@ object Rat {
file("daffodil-sapi/src/test/resources/test/sapi/myData16.dat"),
file("daffodil-sapi/src/test/resources/test/sapi/myData19.dat"),
file("daffodil-sapi/src/test/resources/test/sapi/myDataBroken.dat"),
-
file("daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/parse_int32"),
+
file("daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/ex_int32_parse"),
+
file("daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/orion_command_parse"),
Review comment:
Yes, renamed binary test files to .dat and sorted runtime2 excludes
before sapi excludes.
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala
##########
@@ -260,6 +272,9 @@ class CodeGeneratorState {
|
|// Methods to initialize, parse, and unparse infoset nodes
|
+ |static inline uint8_t be8toh(uint8_t be8b) { return be8b; }
Review comment:
Correct, "be8toh" and company aren't in the standard library and we
still want to generate calls to them even for the degenerate 8-bit (1-byte)
cases.
##########
File path: daffodil-runtime2/src/main/resources/c/libcli/xml_reader.c
##########
@@ -184,10 +184,50 @@ xmlInt32Elem(XMLReader *reader, const ERD *erd, int32_t
*location)
{
if (strcmp(name_from_xml, name_from_erd) == 0)
{
- // Check for any errors getting the 32-bit integer
+ // Check for any errors getting the integer number
const char *errstr = NULL;
- *location = (int32_t)strtonum(number_from_xml, INT32_MIN,
INT32_MAX,
- &errstr);
+
+ // Need to handle varying bit lengths and signedness
Review comment:
Changed to "Handle varying bit lengths of both signed & unsigned
numbers".
##########
File path: daffodil-runtime2/src/main/resources/c/libcli/xml_writer.c
##########
@@ -90,29 +90,64 @@ xmlEndComplex(XMLWriter *writer, const InfosetBase *base)
return complex ? NULL : "Underflowed the XML stack";
}
-// Write 32-bit integer value as element
+// Write 8, 16, 32, or 64-bit signed/unsigned integer as element
static const char *
-xmlInt32Elem(XMLWriter *writer, const ERD *erd, const int32_t *location)
+xmlIntegerElem(XMLWriter *writer, const ERD *erd, const void *intLocation)
{
mxml_node_t *parent = stack_top(&writer->stack);
const char * name = get_erd_name(erd);
- const char * xmlns = get_erd_xmlns(erd);
mxml_node_t *simple = mxmlNewElement(parent, name);
- int32_t value = *location;
- mxml_node_t *text = mxmlNewOpaquef(simple, "%i", value);
+
+ // Set namespace declaration if necessary
+ const char *xmlns = get_erd_xmlns(erd);
if (xmlns)
{
const char *ns = get_erd_ns(erd);
mxmlElementSetAttr(simple, xmlns, ns);
}
- return (simple && text) ? NULL : "Error making new int32 simple element";
+
+ // Need to handle varying bit lengths and signedness
Review comment:
Changed to "Handle varying bit lengths of both signed & unsigned numbers"
##########
File path: daffodil-runtime2/src/main/resources/examples/ex_int32.c
##########
@@ -25,15 +25,15 @@
static void c2_initSelf(c2 *instance);
static const char *c2_parseSelf(c2 *instance, const PState *pstate);
static const char *c2_unparseSelf(const c2 *instance, const UState *ustate);
-static void c1_initSelf(c1 *instance);
-static const char *c1_parseSelf(c1 *instance, const PState *pstate);
-static const char *c1_unparseSelf(const c1 *instance, const UState *ustate);
+static void ex_int32_initSelf(ex_int32 *instance);
Review comment:
This particular example has only signed int32 numbers. Mixing below is
really due to type signatures of functions being called.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
