tuxji commented on a change in pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471#discussion_r557720275
##########
File path: daffodil-runtime2/src/main/resources/c/libcli/xml_reader.c
##########
@@ -97,6 +98,72 @@ strtounum(const char *numptr, uintmax_t maxval, const char
**errstrp)
return value;
}
+// Convert an XML element's text to a float (call strtof with our own
Review comment:
Yes, we would need to move strtofnum and company to libruntime if
daffodil-runtime2 starts supporting text numbers. Right now our [design
notes](https://cwiki.apache.org/confluence/display/DAFFODIL/WIP%3A+Daffodil+Runtime+2)
say dfdl:representation should be only 'binary', not 'text'. We still have to
finish supporting binary representations first and I've already noticed that we
may need to support choices even in binary representations (a tag field
followed by a choice of any equally valid message element type specified by the
tag field's value).
##########
File path: daffodil-runtime2/src/main/resources/c/libcli/xml_reader.c
##########
@@ -17,21 +17,22 @@
#include "xml_reader.h"
#include <errno.h> // for errno
-#include <inttypes.h> // for strtoimax
+#include <inttypes.h> // for strtoimax, strtoumax
Review comment:
Actually, I'm using a program called
[include-what-you-use](https://include-what-you-use.org/) which generates these
comments each time it suggests a change to the headers. I wish IWYU was easier
to install (you have to install clang-11, libclang-11-dev, and llvm-11-dev,
clone the IWYU repo, check out the clang_11 branch, and build and install IWYU
from source code), easier to run (you have to run it individually on each .c
file in a shell for-loop with the same -I flags the Makefile uses), and better
at updating out-of-date comments (you get a set of new comments only when IWYU
tells you to add or remove a header). But I've just updated IWYU to the latest
version, run it on all the C files, and ensured all the comments are up to date
now. I'll include these changes in my next pull request and I think you'll
like the self-documentation of what symbols each file needs in order to do its
work.
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala
##########
@@ -139,26 +159,18 @@ class CodeGeneratorState {
}
def addComplexTypeStatements(child: ElementBase): Unit = {
- val C = child.namedQName.local
+ val C = localName(child)
val e = child.name
val initStatement = s" ${C}_initSelf(&instance->$e);"
- val parseStatement =
- s""" if (!error_msg)
- | {
- | error_msg = ${C}_parseSelf(&instance->$e, pstate);
- | }""".stripMargin
- val unparseStatement =
- s""" if (!error_msg)
- | {
- | error_msg = ${C}_unparseSelf(&instance->$e, ustate);
- | }""".stripMargin
+ val parseStatement = s" ${C}_parseSelf(&instance->$e, pstate);"
+ val unparseStatement = s" ${C}_unparseSelf(&instance->$e, ustate);"
structs.top.initStatements += initStatement
structs.top.parserStatements += parseStatement
structs.top.unparserStatements += unparseStatement
}
def pushComplexElement(context: ElementBase): Unit = {
- val C = context.namedQName.local
+ val C = localName(context)
Review comment:
I haven't been qualifying C struct names yet. I didn't think we would
need to qualify them like we need to qualify C struct field names to avoid name
collisions. Now that I think about it, however, we could have two complex
elements with the same local name in two different namespaces. We would need
to qualify these two complex elements' C struct names with their namespace
prefixes to avoid a name collision. Right now qualifiedName(context) would
only qualify a complex element's local name with the local name of any parent
complex element the complex element is nested inside. Let's address how to
qualify C struct names another time; we may want to treat them differently than
C struct field names anyway.
##########
File path:
daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/orion-command.tdml
##########
@@ -0,0 +1,92 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<tdml:testSuite
+ defaultConfig="config-runtime1"
+ defaultImplementations="daffodil daffodil-runtime2"
+ defaultRoundTrip="onePass"
+ description="TDML tests for orion-command.xml"
+ 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>
+
+ <tdml:parserTestCase
+ description="orion-command Command parse test"
+ model="orion-command.xml"
Review comment:
Yeah. This file came from a collaborator and I'd wanted to keep the
same name, but I'll change the name.
##########
File path:
daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/orion-command.tdml
##########
@@ -0,0 +1,92 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<tdml:testSuite
+ defaultConfig="config-runtime1"
Review comment:
Yes. I've been thinking that I'd like to simplify this boilerplate and
do away with the config-runtime1 and config-runtime2. Initially I thought
Daffodil could look at defaultImplementations="daffodil daffodil-runtime2" and
know it implements both of these, so it might as well run both of them.
However, I've found out that runtime1 prints floating point numbers to XML in a
way that runtime2 can't print to XML compatibly without a lot of extra effort.
Therefore the same XML test data file won't pass in both runtime1 and runtime2.
Still, we might simplify the boilerplate by adding a new attribute that has
the same effect as defaultConfig="config-runtime1" or
defaultConfig="config-runtime2".
##########
File path: daffodil-runtime2/src/main/resources/c/libcli/xml_reader.c
##########
@@ -97,6 +98,72 @@ strtounum(const char *numptr, uintmax_t maxval, const char
**errstrp)
return value;
}
+// Convert an XML element's text to a float (call strtof with our own
+// error checking)
+
+static float
+strtofnum(const char *numptr, const char **errstrp)
Review comment:
I see; that makes sense. Instead of checking whether a const char
*error_msg was returned, we can pass an error_info struct and check whether it
has some error messages afterwards. I don't think Daffodil has
internationalized messages right now; has anyone asked for that support soon?
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala
##########
@@ -139,26 +159,18 @@ class CodeGeneratorState {
}
def addComplexTypeStatements(child: ElementBase): Unit = {
- val C = child.namedQName.local
+ val C = localName(child)
val e = child.name
val initStatement = s" ${C}_initSelf(&instance->$e);"
- val parseStatement =
- s""" if (!error_msg)
- | {
- | error_msg = ${C}_parseSelf(&instance->$e, pstate);
- | }""".stripMargin
- val unparseStatement =
- s""" if (!error_msg)
- | {
- | error_msg = ${C}_unparseSelf(&instance->$e, ustate);
- | }""".stripMargin
+ val parseStatement = s" ${C}_parseSelf(&instance->$e, pstate);"
+ val unparseStatement = s" ${C}_unparseSelf(&instance->$e, ustate);"
Review comment:
Actually, I spotted that CodeCov check myself and realized that I needed
to add nested elements back to my ex_nums test element. I added a second
commit after your review which should ensure these lines are covered by the
ex_nums tests now.
##########
File path:
daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/orion-command.xml
##########
@@ -0,0 +1,80 @@
+<?xml version="1.0"?>
Review comment:
Ditto.
##########
File path: daffodil-runtime2/src/main/resources/c/libruntime/infoset.h
##########
@@ -102,14 +100,16 @@ typedef struct InfosetBase
typedef struct PState
{
- FILE *stream; // input to read from
+ FILE * stream; // input to read from
+ const char *error_msg; // to stop if an error happens
Review comment:
I'm not sure when we will need to support backtracking in a binary
representation with only a limited subset of DFDL 1.0 supported. A
collaborator my group is working with has been using `dfdl:discriminator
test="{../../tagt eq 111}"` on message elements to perform discrimination from
a choice of equally valid message elements, but I think his schema should be
using dfdl:choiceDispatchKey instead which doesn't require backtracking. When
and where might we start to need backtracking, I'm wondering?
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala
##########
@@ -217,10 +232,12 @@ class CodeGeneratorState {
case PrimType.Int => "int32_t "
case PrimType.Short => "int16_t "
case PrimType.Byte => "int8_t "
+ case PrimType.Float => "float "
+ case PrimType.Double => "double "
case x => context.SDE("Unsupported primitive type: " + x)
}
} else {
- child.namedQName.local + " "
+ localName(child)
Review comment:
Hmm, I didn't think we would need to qualify C struct field names since
their names only need to be unique inside each C struct's scope. Are you
thinking somebody might define a complex element with two children (either
simple or complex elements) having the same local name in two different
namespaces too? We would need a prefix_localName in that case too.
----------------------------------------------------------------
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]
