mbeckerle commented on a change in pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471#discussion_r556815398
##########
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:
These comments all say "XML". These library routines have nothing to do
with XML per se.
We may be using this library for converting XML strings into numbers right
now, but as soon as we start implenting DFDL number parsing from text, we'll be
wanting to pull these out and reuse them for that.
Not suggesting we change that now, but just suggesting anticipate a string
library almost just like this in the runtime library.
##########
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:
You might not want to have to maintain these comments saying exactly
what is/isn't used from each include.
It's the kind of thing that is easy to have just become obsolete and then no
longer useful.
##########
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:
Feels like you want qualfiedName(child) here, but you want to convert
that into something you can use as a field name.
At some point we'll have to pick a preferred prefix for a namespace from the
available prefixes for a namespace, and then construct a field name by
prefix_localName or some such formula.
##########
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:
This is the error string vs. object thing again.
This will start to matter when we have backtracking. That's when parse
errors can sometimes be suppressed so you don't want to construct the whole
error string with arguments in it, just to discard it. You just want to point
at the template, and point at the args. And then drop the whole thing when you
backtrack and suppress it.
I am happy with deferring this idea to a subsequent change set though, as
this one is busy/big enough as is.
##########
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:
Something to think about is errors coming back as strings, vs. some
errorInfo object that doesn't get in the way of internationalization. We
probably want an open-ended struct with a string, and a bunch of generic slots
for pointers to values. Then something that wants to display the message can
use the string with format to substitute the arguments.
##########
File path: daffodil-runtime2/src/main/resources/c/libruntime/infoset.c
##########
@@ -136,7 +136,7 @@ walkInfosetNode(const VisitEventHandler *handler, const
InfosetBase *infoNode)
// We use only one of these variables below depending on typeCode
const InfosetBase *childNode =
(const InfosetBase *)((const char *)infoNode + offset);
- const void *intLocation =
+ const void *numLocation =
Review comment:
I was going to suggest why not just call this repLocation, but I think
we'd better design the infoset node for strings, booleans, date/time/datetime,
arrays, etc. etc. Not everything is going to be a pointer to a thing
necessarily.
So no change suggested.
##########
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:
Shouldn't this be qualifiedName(context) ?
##########
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:
file name convention is ".dfdl.xml" or ".dfdl.xsd"
##########
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:
CodeCov check says this is untested, but I find that hard to believe.
This is generating the unparse statement. This has to be called doesn't it?
##########
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:
So if I want to run these tests on runtime2 I change this defaultConfig?
##########
File path:
daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/orion-command.xml
##########
@@ -0,0 +1,80 @@
+<?xml version="1.0"?>
Review comment:
file name convention ".dfdl.xsd" or ".dfdl.xml"
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala
##########
@@ -247,10 +264,12 @@ class CodeGeneratorState {
val erds = this.erds.mkString("\n")
val finalImplementation = this.finalImplementation.mkString("\n")
val code =
- s"""#include "generated_code.h" // for generated code structs
- |#include <endian.h> // for be32toh, htobe32, etc.
- |#include <stddef.h> // for ptrdiff_t
- |#include <stdio.h> // for NULL, fread, fwrite, size_t, FILE
+ s"""#define __STDC_WANT_IEC_60559_BFP_EXT__
Review comment:
Comment needed to explain what WANT_IEC_60559_BFP means. I think BFP
stands for binary floating point?
----------------------------------------------------------------
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]
