You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2021/01/14 22:52:17 UTC
[GitHub] [incubator-daffodil] tuxji commented on a change in pull request #471: Support floating point numbers
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:
users@infra.apache.org