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/13 17:07:40 UTC
[GitHub] [incubator-daffodil] tuxji opened a new pull request #471: Support floating point numbers
tuxji opened a new pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471
Expand the C code generator to support binary floating numbers.
Rename test element ex_ints to ex_nums and add floating numbers to it.
Modify the C code generator to emit one-line parser and unparser
subroutine calls instead of emitting 14-16 lines of low level parsing
/ unparsing C code for each field. Define each variation of these
14-16 lines for each type of numerical field as subroutines inside
parsers.c and unparsers.c in the c/libruntime directory instead.
Move the orion-command test element from TestRuntime2.dfdl.xsd to its
own schema (orion-command.xml) with its own tests and data files.
Currently orion-command.tdml will pass using daffodil-runtime1, but
fail if using daffodil-runtime2. We need to make the C code generator
support array elements in orion-command.xml's VideoSettings element.
Changelog:
In TestCLIGenerateC.scala, rename ex_ints to ex_nums.
In daffodil_main.c, update the parseSelf and unparseSelf calls since
they now return void instead of const char *error_msg.
In xml_reader.c, define new strtofnum and strtodnum functions to
support reading floating point numbers from XML data. Rename
xmlIntegerElem to xmlNumberElem and make it call strtofnum and
strtodnum as well as strtounum and strtonum. Run clang-format on the
code.
In xml_writer.c, rename xmlIntegerElem to xmlNumberElem and make it
write floating point numbers as well as integers. Run clang-format on
the code.
In infoset.c, make walkInfosetNode call visitNumberElem for primitive
floats/doubles as well as primitive integers.
In infoset.h, change signatures of parseSelf and unparseSelf. Rename
VisitIntegerElem to VisitNumberElem. Add enumerations for primitive
floats/doubles. Add const char *error_msg to PState and UState
structs so parser/unparser subroutines can set it when needed.
Add parsers.{c,h} with subroutines to parse binary floating point
numbers and integers.
Add unparsers.{c,h} with subroutines to unparse binary floating point
numbers and integers.
Rename ex_ints.{c,h} to ex_nums.{c,h} and bring them up to date with
generated_code.{c,h} emitted by C code generator after latest changes.
In Runtime2CodeGenerator.scala, add BinaryFloat and BinaryDouble cases
within the generateCode function and make them call
BinaryFloatCodeGenerator. Also add and call a noop function to
facilitate setting a breakpoint within the generateCode function.
Add BinaryFloatCodeGenerator.scala and make it emit one-line
subroutine calls for parsing and unparsing binary floating point
numbers. Note that binary floating point numbers can be little-endian
or big-endian just like binary integers.
In BinaryIntegerKnownLengthCodeGenerator.scala, remove minimum 8-bit
alignment restriction since C code can read and write numbers without
needing alignment to 8-bit boundaries. Change to emit one-line parse
and unparse subroutine calls (the low level parsing and unparsing code
is in c/libruntime/parsers.c and unparsers.c now).
In CodeGeneratorState.scala, add qualifiedName and localName methods
and invoke them to generate unique C file-scope names for ERD objects
while continuing to generate non-unique field names. Support binary
floating numbers as well as binary integers and initialize their
fields with signaling NaNs. Change parseSelf and unparseSelf to
return void instead of const char *error_msg. Expand to handle floats
as well as integers. Shorten ${C}_compute_ERD_offsets to
${C}_compute_offsets. Remove unneeded headers from generated_code.c.
In TestRuntime2.dfdl.xsd, rename ex_ints to ex_nums and make it define
float/double numbers as well as integers. Move orion_command and its
simple types to orion-command.xml.
In TestRuntime2.tdml, rename ex_ints tests to ex_nums. Move
orion_command tests to orion-command.tdml.
Rename orion_command_parse.dat to command_parse.dat.
Rename orion_command_unparse.xml to command_unparse.xml.
Rename ex_ints test data files to ex_nums and add float/double numbers
to their contents.
Add orion-command.tdml with test cases for Command and VideoSettings
elements and corresponding data files (e.g., video_settings_parse.dat
and video_settings_unparse.xml).
Add orion-command.xml schema with Command, CameraState, and
VideoSettings elements.
Add TestOrionCommand.scala to run orion-command.tdml as part of sbt
tests.
In TestRuntime2.scala, rename ex_ints to ex_nums. Move orion_command
test cases to TestOrionCommand.scala.
In Dependencies.scala, update dev.dirs:directories version to latest
version.
In Rat.scala, tell Rat to ignore new test data files. Also ignore
orion-command.xml schema since we got it from a third party without
any license (we may keep it around only temporarily while developing C
code generator and remove it once Daffodil can generate C code for
array elements).
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] tuxji commented on a change in pull request #471: Support floating point numbers
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471#discussion_r557742742
##########
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 file scope static ERD 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 ERD object names anyway.
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #471: Support floating point numbers
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471#discussion_r562248050
##########
File path: daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/BinaryIntegerKnownLengthCodeGenerator.scala
##########
@@ -18,70 +18,52 @@
package org.apache.daffodil.runtime2.generators
import org.apache.daffodil.grammar.primitives.BinaryIntegerKnownLength
+import org.apache.daffodil.schema.annotation.props.gen.OccursCountKind
import org.apache.daffodil.schema.annotation.props.gen.{ BitOrder, ByteOrder }
trait BinaryIntegerKnownLengthCodeGenerator {
def binaryIntegerKnownLengthGenerateCode(g: BinaryIntegerKnownLength, cgState: CodeGeneratorState): Unit = {
// For the time being this is a very limited back end.
- // So there are numerous restrictions to enforce.
+ // So there are some restrictions to enforce.
val e = g.e
- val fieldName = e.namedQName.local
val lengthInBits: Long = {
e.schemaDefinitionUnless(e.elementLengthInBitsEv.isConstant, "Runtime dfdl:length expressions not supported.")
val len = e.elementLengthInBitsEv.constValue.get
len
}
-
+ if (e.bitOrder ne BitOrder.MostSignificantBitFirst)
+ e.SDE("Only dfdl:bitOrder 'mostSignificantBitFirst' is supported.")
Review comment:
If you want to get rid of this codecov annotation about this line, you can use
e.schemaDefinitionWhen(....)
That embeds the predicate into the same line of code, so the predicate gets executed, which will satisfy codecov.
We need a shorter alias though, should be SDEWhen(predicate, msgTemplate, args*)
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] tuxji merged pull request #471: Support arrays, real numbers, and improve TDML processing
Posted by GitBox <gi...@apache.org>.
tuxji merged pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #471: Support floating point numbers
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471#discussion_r558551643
##########
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:
One option might be to add type awareness to the TDML runner. We already have that for calendars, hex binary, and blobs (see ``def textIsSame`` in ``XMLUtils.scala``). We could make it so if an infoset includes ``type="xs:float"`` or ``xs:double``, then we consider float values to be the same if they are within some small delta.
Or I wonder if the DFDL spec provides any guidance on how floating point values should be converted to base-10 strings for infoset? Maybe that's outside the scope of DFDL. Perhaps we should create a bug to make sure different runtimes output same floating point string reps? I assume runtime1 just uses float .toString() so we're just relying on Java to do the right thing, but maybe it's not doing the best thing in all cases.
##########
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:
You mention that this orion files do not have a license? Can we get the contributor to license them as Apache? If not, I'd prefer we try to keep non-Apache compliant licenses out of the repo, even if they are on branches--it's too easy to forget about them once things get merged. At the very least, we need a scary comment somewhere (maybe Rat.scala) that these files are only temporarily ignored and do not follow apache license guidelines.
I'd also rather strive to get those binary files in TDML files. It's just easier to visiual the data that way and verify a test without having to fire up a hex editor to view the bin files.
##########
File path: daffodil-runtime2/src/main/resources/c/libruntime/parsers.h
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.
+ */
+
+#ifndef PARSERS_H
+#define PARSERS_H
+
+#include "infoset.h" // for PState
+#include <stdint.h> // for uint64_t, int32_t, etc.
+
+// Functions to parse binary floating point numbers and integers
+
+extern void parse_be_double(double *number, PState *pstate);
+extern void parse_be_float(float *number, PState *pstate);
+
+extern void parse_be_uint64(uint64_t *number, PState *pstate);
+extern void parse_be_uint32(uint32_t *number, PState *pstate);
+extern void parse_be_uint16(uint16_t *number, PState *pstate);
+extern void parse_be_uint8(uint8_t *number, PState *pstate);
+
+extern void parse_be_int64(int64_t *number, PState *pstate);
+extern void parse_be_int32(int32_t *number, PState *pstate);
+extern void parse_be_int16(int16_t *number, PState *pstate);
+extern void parse_be_int8(int8_t *number, PState *pstate);
+
+extern void parse_le_double(double *number, PState *pstate);
+extern void parse_le_float(float *number, PState *pstate);
+
+extern void parse_le_uint64(uint64_t *number, PState *pstate);
+extern void parse_le_uint32(uint32_t *number, PState *pstate);
+extern void parse_le_uint16(uint16_t *number, PState *pstate);
+extern void parse_le_uint8(uint8_t *number, PState *pstate);
+
+extern void parse_le_int64(int64_t *number, PState *pstate);
+extern void parse_le_int32(int32_t *number, PState *pstate);
+extern void parse_le_int16(int16_t *number, PState *pstate);
+extern void parse_le_int8(int8_t *number, PState *pstate);
+
+#endif // PARSERS_H
Review comment:
There's so much duplication in {paresrs,unparsers}.{c,h}. Any thoughts on using macros to reduce this?
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] tuxji commented on a change in pull request #471: Support floating point numbers
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [incubator-daffodil] tuxji commented on a change in pull request #471: Support floating point numbers
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471#discussion_r559697437
##########
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:
I've split ex_nums_unparse.xml into ex_nums_unparse1.xml from runtime1 and ex_nums_unparse2.xml from runtime2 to illustrate the differences in XML floating point output between runtime1 and runtime2. I'll start a discussion on the dev mailing list on how to resolve these differences. One way would be to make the TDML runner use numerical comparison instead of textual comparison for floats/doubles to abstract these differences away as you suggest. Another way would be to make runtime1 and runtime2 match each other without breaking existing external or internal code/tests/usage.
##########
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:
I'm still waiting for a reply to an email I sent our collaborator last week whether we can license orion-command.xml as Apache. I see the point about moving binary data into TDML files, On the other hand having binary data files makes it easier to debug C code generated by the C code generator (compile with -g and run under the debugger with the binary data file as a command line argument).
##########
File path: daffodil-runtime2/src/main/resources/c/libruntime/parsers.h
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.
+ */
+
+#ifndef PARSERS_H
+#define PARSERS_H
+
+#include "infoset.h" // for PState
+#include <stdint.h> // for uint64_t, int32_t, etc.
+
+// Functions to parse binary floating point numbers and integers
+
+extern void parse_be_double(double *number, PState *pstate);
+extern void parse_be_float(float *number, PState *pstate);
+
+extern void parse_be_uint64(uint64_t *number, PState *pstate);
+extern void parse_be_uint32(uint32_t *number, PState *pstate);
+extern void parse_be_uint16(uint16_t *number, PState *pstate);
+extern void parse_be_uint8(uint8_t *number, PState *pstate);
+
+extern void parse_be_int64(int64_t *number, PState *pstate);
+extern void parse_be_int32(int32_t *number, PState *pstate);
+extern void parse_be_int16(int16_t *number, PState *pstate);
+extern void parse_be_int8(int8_t *number, PState *pstate);
+
+extern void parse_le_double(double *number, PState *pstate);
+extern void parse_le_float(float *number, PState *pstate);
+
+extern void parse_le_uint64(uint64_t *number, PState *pstate);
+extern void parse_le_uint32(uint32_t *number, PState *pstate);
+extern void parse_le_uint16(uint16_t *number, PState *pstate);
+extern void parse_le_uint8(uint8_t *number, PState *pstate);
+
+extern void parse_le_int64(int64_t *number, PState *pstate);
+extern void parse_le_int32(int32_t *number, PState *pstate);
+extern void parse_le_int16(int16_t *number, PState *pstate);
+extern void parse_le_int8(int8_t *number, PState *pstate);
+
+#endif // PARSERS_H
Review comment:
I've added "use macros to achieve DRY (Don't Repeat Yourself)" in these files to my TODO list for my next commit.
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] tuxji commented on a change in pull request #471: Support floating point numbers
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471#discussion_r558327251
##########
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:
I needed this WANT_IEC_60559_BFP to make <math.h> provide the signaling NaN macros (SNAN, SNANF), but I ended up removing that define in my second commit because it didn't work on Windows. I decided it wasn't worth trying to initialize fields with SNAN/SNANF (too unportable) and initialized them with NAN instead (a more portable macro).
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #471: Support floating point numbers
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471#discussion_r562248050
##########
File path: daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/BinaryIntegerKnownLengthCodeGenerator.scala
##########
@@ -18,70 +18,52 @@
package org.apache.daffodil.runtime2.generators
import org.apache.daffodil.grammar.primitives.BinaryIntegerKnownLength
+import org.apache.daffodil.schema.annotation.props.gen.OccursCountKind
import org.apache.daffodil.schema.annotation.props.gen.{ BitOrder, ByteOrder }
trait BinaryIntegerKnownLengthCodeGenerator {
def binaryIntegerKnownLengthGenerateCode(g: BinaryIntegerKnownLength, cgState: CodeGeneratorState): Unit = {
// For the time being this is a very limited back end.
- // So there are numerous restrictions to enforce.
+ // So there are some restrictions to enforce.
val e = g.e
- val fieldName = e.namedQName.local
val lengthInBits: Long = {
e.schemaDefinitionUnless(e.elementLengthInBitsEv.isConstant, "Runtime dfdl:length expressions not supported.")
val len = e.elementLengthInBitsEv.constValue.get
len
}
-
+ if (e.bitOrder ne BitOrder.MostSignificantBitFirst)
+ e.SDE("Only dfdl:bitOrder 'mostSignificantBitFirst' is supported.")
Review comment:
If you want to get rid of this codecov annotation about this line, you can use
e.schemaDefinitionWhen(....)
That embeds the predicate into the same line of code, so the predicate gets executed, which will satisfy codecov.
We need a shorter alias though, should be SDEWhen(predicate, msgTemplate, args*)
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] stevedlawrence commented on pull request #471: Support floating point numbers
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471#issuecomment-761161770
+1 with minor comments. Would like to also see the windows issues fixed, but I think based on another comment you fixed the portability already.
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #471: Support floating point numbers
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] tuxji merged pull request #471: Support arrays, real numbers, and improve TDML processing
Posted by GitBox <gi...@apache.org>.
tuxji merged pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #471: Support floating point numbers
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #471:
URL: https://github.com/apache/incubator-daffodil/pull/471#discussion_r558537992
##########
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:
Internationalized message support will eventually come up. It's very painful unless we put the support in early. This is also an issue of performance for backtracking (eventually someday)
##########
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:
Not soon. My preference is create the deterministic DFDL subset first. Eventually people will want a fully functioning back-end.
##########
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:
The other reason to do this is so that programmers won't write code assembling error messages from strings. They'll just pass a bunch of static pointers. This is less code clutter.
----------------------------------------------------------------
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