[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #471: Support floating point numbers

[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