stevedlawrence commented on a change in pull request #650:
URL: https://github.com/apache/daffodil/pull/650#discussion_r721322695
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/xml_reader.c
##########
@@ -341,7 +416,7 @@ xmlNumberElem(XMLReader *reader, const ERD *erd, void
*number)
reader->node = mxmlWalkNext(reader->node, reader->xml, MXML_DESCEND);
// Check whether we are walking both XML data and infoset in lockstep
- if (name_from_xml && name_from_erd)
+ if (name_from_xml && name_from_erd && number_from_xml)
Review comment:
Is it worth renaming `number_from_xml` to `value_from_xml` or something
to make it clear that this isn't necessarily a number anymore?
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/xml_reader.c
##########
@@ -217,6 +217,81 @@ strtounum(const char *numptr, uintmax_t maxval, const
Error **errorptr)
return value;
}
+// Store an XML element's string of hexadecimal characters (two
+// nibbles per byte) into a byte array. Allocate memory for byte
+// array if needed. Return error if string does not fit into byte
+// array or does not contain valid hexadecimal characters.
+
+static const Error *
+hexToBinary(const char *hexstring, HexBinary *hexBinary)
+{
+ // Check whether string has even number of nibbles
+ size_t numNibbles = strlen(hexstring);
+ size_t numBytes = numNibbles / 2;
+ if (numNibbles == 0 || (numNibbles % 2) != 0)
Review comment:
It is valid in DFDL to have a zero-length hex binary element. Any
thoughts on allowing this? Maybe it makes logic a bit more complicated since
`malloc(0)` can return null so you'd need to deal with `hexBinary->array`
allowed to be NULL.
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/infoset.h
##########
@@ -91,6 +94,15 @@ typedef struct ERD
const InitChoiceRD initChoice;
} ERD;
+// HexBinary - parameters of a hexBinary element
+
+typedef struct HexBinary
+{
+ uint8_t *array; // pointer to element's byte array
+ size_t size; // size of element's byte array (in bytes)
Review comment:
It looks like this comment isn't 100% accurate. It's not the size of the
array, but it's the number of bytes in array that make up the hex binary. The
actually allocated capacity of the array could be larger. In fact, I wonder if
it's worth adding a capacity element to differentiate the two?
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/unparsers.c
##########
@@ -133,10 +133,18 @@ unparse_fill_bytes(size_t end_position, const char
fill_byte, UState *ustate)
while (ustate->position < end_position)
{
- write_stream_update_position;
+ write_stream_update_position(buffer.c_val, 1);
}
}
+// Unparse 8-bit bytes from hexBinary field
+
+void
+unparse_hexBinary(HexBinary hexBinary, UState *ustate)
Review comment:
Is there a convention you use for determining when to use a pointer or
not for a paremter? I ask because `parse_hexBinary` uses a `HexBinary` pointer
but this does not.
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/HexBinaryCodeGenerator.scala
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+package org.apache.daffodil.runtime2.generators
+
+import org.apache.daffodil.dpath.NodeInfo.PrimType
+import org.apache.daffodil.grammar.primitives.HexBinaryLengthPrefixed
+import org.apache.daffodil.grammar.primitives.HexBinarySpecifiedLength
+import org.apache.daffodil.schema.annotation.props.gen.BitOrder
+import org.apache.daffodil.schema.annotation.props.gen.ByteOrder
+import org.apache.daffodil.schema.annotation.props.gen.OccursCountKind
+
+trait HexBinaryCodeGenerator extends BinaryAbstractCodeGenerator {
+
+ def hexBinaryLengthPrefixedGenerateCode(g: HexBinaryLengthPrefixed, cgState:
CodeGeneratorState): Unit = {
+ // For the time being this is a very limited back end.
+ // So there are some restrictions to enforce.
+ val e = g.e
+ e.schemaDefinitionUnless(e.bitOrder eq BitOrder.MostSignificantBitFirst,
"Only dfdl:bitOrder 'mostSignificantBitFirst' is supported.")
+
+ val fieldName = e.namedQName.local
+ val arraySize = if (e.occursCountKind == OccursCountKind.Fixed)
e.maxOccurs else 0
+ val conv = if (e.prefixedLengthElementDecl.byteOrderEv.constValue ==
ByteOrder.BigEndian) "be" else "le"
+ val intType = e.prefixedLengthElementDecl.optPrimType.get match {
+ case PrimType.Byte
+ | PrimType.Short
+ | PrimType.Int
+ | PrimType.Long
+ | PrimType.Integer => "int"
+ case PrimType.UnsignedByte
+ | PrimType.UnsignedShort
+ | PrimType.UnsignedInt
+ | PrimType.UnsignedLong
+ | PrimType.NonNegativeInteger => "uint"
+ case p => e.SDE("Prefixed length PrimType %s is not supported in C code
generator.", p.toString)
+ }
+ val intLen =
e.prefixedLengthElementDecl.elementLengthInBitsEv.constValue.get
+
+ def addStatements(deref: String): Unit = {
+ val field = s"instance->$fieldName$deref"
+ val i = if (deref.length > 2) deref.substring(1, deref.length - 1) else
""
+ val lenVar = s"_l_$fieldName$i"
+ val initStatement =
+ s""" $field.array = NULL;
+ | $field.size = 0;
+ | $field.dynamic = true;""".stripMargin
+ val parseStatement =
+ s""" $intType${intLen}_t $lenVar;
+ | parse_${conv}_$intType$intLen(&$lenVar, pstate);
+ | if (pstate->error) return;
+ | parse_alloc(&$field, $lenVar, pstate);
+ | if (pstate->error) return;
+ | parse_hexBinary(&$field, pstate);
+ | if (pstate->error) return;""".stripMargin
+ val unparseStatement =
+ s""" unparse_${conv}_$intType$intLen($field.size, ustate);
+ | if (ustate->error) return;
+ | unparse_hexBinary($field, ustate);
+ | if (ustate->error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(initStatement, parseStatement,
unparseStatement)
+
+ if (e.hasFixedValue) {
+ val variable = s"${fieldName}_fixed$i"
+ val value = e.fixedValueAsString.grouped(2).mkString(", 0x")
+ val init2 = ""
+ val parse2 =
+ s""" uint8_t $variable[] = {0x$value};
+ | parse_validate_fixed(memcmp($field.array, $variable,
sizeof($variable)) == 0, "$fieldName", pstate);
+ | if (pstate->error) return;""".stripMargin
+ val unparse2 =
+ s""" uint8_t $variable[] = {0x$value};
+ | unparse_validate_fixed(memcmp($field.array, $variable,
sizeof($variable)) == 0, "$fieldName", ustate);
+ | if (ustate->error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(init2, parse2, unparse2)
+ }
+ }
+ if (arraySize > 0)
+ for (i <- 0 until arraySize)
+ addStatements(s"[$i]")
+ else
+ addStatements("")
+ }
+
+ def hexBinarySpecifiedLengthGenerateCode(g: HexBinarySpecifiedLength,
cgState: CodeGeneratorState): Unit = {
+ // For the time being this is a very limited back end.
+ // So there are some restrictions to enforce.
+ val e = g.e
+ e.schemaDefinitionUnless(e.bitOrder eq BitOrder.MostSignificantBitFirst,
"Only dfdl:bitOrder 'mostSignificantBitFirst' is supported.")
+ e.schemaDefinitionUnless(e.elementLengthInBitsEv.isConstant, "Runtime
dfdl:length expressions not supported.")
+
+ val fieldName = e.namedQName.local
+ val arraySize = if (e.occursCountKind == OccursCountKind.Fixed)
e.maxOccurs else 0
Review comment:
You might consider a different check for `arraySize`? It seems like we
support either fixed length arrays or scalars. Maybe something like this
instead:
```scala
val arraySize = e.occursCountKind match {
case OccursCountKind.Fixed if e.maxOccurs > 1 => e.maxOccurs
case OccursCountKind.Fixed if e.maxOccurs == 1 => 0
case OccursCountKind.Implicit if e.minOccurs == 1 && e.maxOccurs == 1 => 0
case _ => SDE("unsupported occursCountKind ...")
}
```
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/HexBinaryCodeGenerator.scala
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+package org.apache.daffodil.runtime2.generators
+
+import org.apache.daffodil.dpath.NodeInfo.PrimType
+import org.apache.daffodil.grammar.primitives.HexBinaryLengthPrefixed
+import org.apache.daffodil.grammar.primitives.HexBinarySpecifiedLength
+import org.apache.daffodil.schema.annotation.props.gen.BitOrder
+import org.apache.daffodil.schema.annotation.props.gen.ByteOrder
+import org.apache.daffodil.schema.annotation.props.gen.OccursCountKind
+
+trait HexBinaryCodeGenerator extends BinaryAbstractCodeGenerator {
+
+ def hexBinaryLengthPrefixedGenerateCode(g: HexBinaryLengthPrefixed, cgState:
CodeGeneratorState): Unit = {
+ // For the time being this is a very limited back end.
+ // So there are some restrictions to enforce.
+ val e = g.e
+ e.schemaDefinitionUnless(e.bitOrder eq BitOrder.MostSignificantBitFirst,
"Only dfdl:bitOrder 'mostSignificantBitFirst' is supported.")
+
+ val fieldName = e.namedQName.local
+ val arraySize = if (e.occursCountKind == OccursCountKind.Fixed)
e.maxOccurs else 0
+ val conv = if (e.prefixedLengthElementDecl.byteOrderEv.constValue ==
ByteOrder.BigEndian) "be" else "le"
+ val intType = e.prefixedLengthElementDecl.optPrimType.get match {
+ case PrimType.Byte
+ | PrimType.Short
+ | PrimType.Int
+ | PrimType.Long
+ | PrimType.Integer => "int"
+ case PrimType.UnsignedByte
+ | PrimType.UnsignedShort
+ | PrimType.UnsignedInt
+ | PrimType.UnsignedLong
+ | PrimType.NonNegativeInteger => "uint"
+ case p => e.SDE("Prefixed length PrimType %s is not supported in C code
generator.", p.toString)
+ }
+ val intLen =
e.prefixedLengthElementDecl.elementLengthInBitsEv.constValue.get
+
+ def addStatements(deref: String): Unit = {
+ val field = s"instance->$fieldName$deref"
+ val i = if (deref.length > 2) deref.substring(1, deref.length - 1) else
""
+ val lenVar = s"_l_$fieldName$i"
+ val initStatement =
+ s""" $field.array = NULL;
+ | $field.size = 0;
+ | $field.dynamic = true;""".stripMargin
+ val parseStatement =
+ s""" $intType${intLen}_t $lenVar;
+ | parse_${conv}_$intType$intLen(&$lenVar, pstate);
+ | if (pstate->error) return;
+ | parse_alloc(&$field, $lenVar, pstate);
+ | if (pstate->error) return;
+ | parse_hexBinary(&$field, pstate);
+ | if (pstate->error) return;""".stripMargin
+ val unparseStatement =
+ s""" unparse_${conv}_$intType$intLen($field.size, ustate);
+ | if (ustate->error) return;
+ | unparse_hexBinary($field, ustate);
+ | if (ustate->error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(initStatement, parseStatement,
unparseStatement)
+
+ if (e.hasFixedValue) {
+ val variable = s"${fieldName}_fixed$i"
+ val value = e.fixedValueAsString.grouped(2).mkString(", 0x")
Review comment:
Suggest doing something like `.mkString("0x", ", 0x", "")` and change
`0x$value` below to just `$value`. Put's the logic of inserting the 0x's in a
single place.
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/xml_writer.c
##########
@@ -108,26 +109,65 @@ xmlEndComplex(XMLWriter *writer, const InfosetBase *base)
static void
fixNumberIfNeeded(const char *text)
{
- if (text[0] == 'N' && text[1] == 'A')
- {
- // xsd:float requires NaN to be capitalized correctly
- char *modifyInPlace = (char *)text;
- modifyInPlace[1] = 'a';
- }
// We aren't writing code to remove all textual differences
// between runtime1/runtime2 because it would be better to make
// Daffodil compare numbers numerically, not textually. If we did
// have to match runtime1 perfectly, we would have to:
// - Strip + from <f>E+<e> to get <f>E<e>
// - Add .0 to 1 to get 1.0
// - Change number of significant digits to match runtime1
+ if (text && text[0] == 'N' && text[1] == 'A')
+ {
+ // xsd:float requires NaN to be capitalized correctly
+ char *modifyInPlace = (char *)text;
+ modifyInPlace[1] = 'a';
+ }
+}
+
+// Convert a byte array to a string of hexadecimal characters (two
+// nibbles per byte). Return NULL if no dynamic memory could be
+// allocated for string. Reuse same dynamic memory next time if big
+// enough. Caller is responsible for copying string before next call.
+
+static const char *
+binaryToHex(HexBinary hexBinary)
+{
+ static char * hexString = NULL;
+ static size_t capacity = 256;
+
+ // Make room for hexadecimal characters if necessary
+ size_t numNibbles = hexBinary.size * 2;
+ if (hexString == NULL || capacity < numNibbles)
+ {
+ free(hexString);
+ capacity = numNibbles > capacity ? numNibbles : capacity;
+ hexString = malloc(capacity + 1);
+ }
+
+ // Check whether dynamic memory allocation succeeded
+ if (hexString == NULL)
+ {
+ return NULL;
+ }
+
+ // Convert each binary byte to two hexadecimal characters
+ char *nibble = hexString;
+ for (size_t i = 0; i < hexBinary.size; i++)
+ {
+ static char hexDigit[] = "0123456789ABCDEF";
+ *(nibble++) = hexDigit[hexBinary.array[i] / 16]; // high nibble
+ *(nibble++) = hexDigit[hexBinary.array[i] % 16]; // low nibble
+ }
+ *(nibble++) = '\0';
+
+ return hexString;
}
// Write a boolean, 32-bit or 64-bit real number, or 8, 16, 32, or
// 64-bit signed or unsigned integer as an XML element's value
static const Error *
-xmlNumberElem(XMLWriter *writer, const ERD *erd, const void *number)
+xmlSimpleElem(XMLWriter *writer, const ERD *erd, const void *number)
Review comment:
Same comments as the reader, should `number` be replaced with something
like `value`?
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/xml_reader.c
##########
@@ -361,6 +436,9 @@ xmlNumberElem(XMLReader *reader, const ERD *erd, void
*number)
case PRIMITIVE_DOUBLE:
*(double *)number = strtodnum(number_from_xml, &error);
return error;
+ case PRIMITIVE_HEXBINARY:
+ error = hexToBinary(number_from_xml, (HexBinary *)number);
Review comment:
This function name and parameters/return value isn't consistent with the
other `strtoX` functions. Why not `strtohexbinary`? Also, the `strtoX`
functions return a value and has an error out paramater, whereas this returns
an error and has a value out parameter? Should these all match? I personally
prefer returning error codes like `hexToBinary`. Thoughts on updated things to
match?
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/xml_writer.c
##########
@@ -108,26 +109,65 @@ xmlEndComplex(XMLWriter *writer, const InfosetBase *base)
static void
fixNumberIfNeeded(const char *text)
{
- if (text[0] == 'N' && text[1] == 'A')
- {
- // xsd:float requires NaN to be capitalized correctly
- char *modifyInPlace = (char *)text;
- modifyInPlace[1] = 'a';
- }
// We aren't writing code to remove all textual differences
// between runtime1/runtime2 because it would be better to make
// Daffodil compare numbers numerically, not textually. If we did
// have to match runtime1 perfectly, we would have to:
// - Strip + from <f>E+<e> to get <f>E<e>
// - Add .0 to 1 to get 1.0
// - Change number of significant digits to match runtime1
+ if (text && text[0] == 'N' && text[1] == 'A')
+ {
+ // xsd:float requires NaN to be capitalized correctly
+ char *modifyInPlace = (char *)text;
+ modifyInPlace[1] = 'a';
+ }
+}
+
+// Convert a byte array to a string of hexadecimal characters (two
+// nibbles per byte). Return NULL if no dynamic memory could be
+// allocated for string. Reuse same dynamic memory next time if big
+// enough. Caller is responsible for copying string before next call.
+
+static const char *
+binaryToHex(HexBinary hexBinary)
+{
+ static char * hexString = NULL;
+ static size_t capacity = 256;
Review comment:
Any concerns with this static variables that you might get a single very
large hex binary blob that will allocate a large chunk of memory that will
never be freed?
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/xml_writer.c
##########
@@ -159,6 +199,9 @@ xmlNumberElem(XMLWriter *writer, const ERD *erd, const void
*number)
text = mxmlNewOpaquef(simple, "%.17lG", *(const double *)number);
fixNumberIfNeeded(mxmlGetOpaque(text));
break;
+ case PRIMITIVE_HEXBINARY:
+ text = mxmlNewOpaque(simple, binaryToHex(*(const HexBinary *)number));
Review comment:
`binaryToHex` can return `NULL` if the `malloc` fails. Should this be
checking for that and return `ERR_HEXBINARY_ALLOC` or something?
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/HexBinaryCodeGenerator.scala
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+package org.apache.daffodil.runtime2.generators
+
+import org.apache.daffodil.dpath.NodeInfo.PrimType
+import org.apache.daffodil.grammar.primitives.HexBinaryLengthPrefixed
+import org.apache.daffodil.grammar.primitives.HexBinarySpecifiedLength
+import org.apache.daffodil.schema.annotation.props.gen.BitOrder
+import org.apache.daffodil.schema.annotation.props.gen.ByteOrder
+import org.apache.daffodil.schema.annotation.props.gen.OccursCountKind
+
+trait HexBinaryCodeGenerator extends BinaryAbstractCodeGenerator {
+
+ def hexBinaryLengthPrefixedGenerateCode(g: HexBinaryLengthPrefixed, cgState:
CodeGeneratorState): Unit = {
+ // For the time being this is a very limited back end.
+ // So there are some restrictions to enforce.
+ val e = g.e
+ e.schemaDefinitionUnless(e.bitOrder eq BitOrder.MostSignificantBitFirst,
"Only dfdl:bitOrder 'mostSignificantBitFirst' is supported.")
+
+ val fieldName = e.namedQName.local
+ val arraySize = if (e.occursCountKind == OccursCountKind.Fixed)
e.maxOccurs else 0
+ val conv = if (e.prefixedLengthElementDecl.byteOrderEv.constValue ==
ByteOrder.BigEndian) "be" else "le"
+ val intType = e.prefixedLengthElementDecl.optPrimType.get match {
+ case PrimType.Byte
+ | PrimType.Short
+ | PrimType.Int
+ | PrimType.Long
+ | PrimType.Integer => "int"
+ case PrimType.UnsignedByte
+ | PrimType.UnsignedShort
+ | PrimType.UnsignedInt
+ | PrimType.UnsignedLong
+ | PrimType.NonNegativeInteger => "uint"
+ case p => e.SDE("Prefixed length PrimType %s is not supported in C code
generator.", p.toString)
+ }
+ val intLen =
e.prefixedLengthElementDecl.elementLengthInBitsEv.constValue.get
+
+ def addStatements(deref: String): Unit = {
+ val field = s"instance->$fieldName$deref"
+ val i = if (deref.length > 2) deref.substring(1, deref.length - 1) else
""
+ val lenVar = s"_l_$fieldName$i"
+ val initStatement =
+ s""" $field.array = NULL;
+ | $field.size = 0;
+ | $field.dynamic = true;""".stripMargin
+ val parseStatement =
+ s""" $intType${intLen}_t $lenVar;
+ | parse_${conv}_$intType$intLen(&$lenVar, pstate);
+ | if (pstate->error) return;
+ | parse_alloc(&$field, $lenVar, pstate);
+ | if (pstate->error) return;
+ | parse_hexBinary(&$field, pstate);
+ | if (pstate->error) return;""".stripMargin
+ val unparseStatement =
+ s""" unparse_${conv}_$intType$intLen($field.size, ustate);
+ | if (ustate->error) return;
+ | unparse_hexBinary($field, ustate);
+ | if (ustate->error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(initStatement, parseStatement,
unparseStatement)
+
+ if (e.hasFixedValue) {
+ val variable = s"${fieldName}_fixed$i"
+ val value = e.fixedValueAsString.grouped(2).mkString(", 0x")
Review comment:
Is a test needed to make sure fixedValueAsString has a multiple of two
length?
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/xml_reader.c
##########
@@ -326,7 +401,7 @@ xmlEndComplex(XMLReader *reader, const InfosetBase *base)
// 64-bit signed or unsigned integer from XML data
static const Error *
-xmlNumberElem(XMLReader *reader, const ERD *erd, void *number)
+xmlSimpleElem(XMLReader *reader, const ERD *erd, void *number)
Review comment:
Should re rename `number` to `value` or something more generic?
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/parsers.c
##########
@@ -149,10 +150,47 @@ parse_fill_bytes(size_t end_position, PState *pstate)
while (pstate->position < end_position)
{
- read_stream_update_position;
+ read_stream_update_position(&buffer.c_val, 1);
}
}
+// Allocate or reallocate memory for hexBinary field
+
+void
+parse_alloc(HexBinary *hexBinary, size_t num_bytes, PState *pstate)
Review comment:
Thoughts on renaming to somethign like parse_hexBinary_alloc or
something to make it clear this is allocating a hexBinary thing? I imagine
we'll eventually have some other kinds of allocations needed?
##########
File path:
daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/ex_nums_unparse_runtime2.xml
##########
@@ -62,11 +67,14 @@
<le_uint64>64</le_uint64>
<le_uint8>8</le_uint8>
<le_nonNegativeInteger8>8</le_nonNegativeInteger8>
+ <hexBinary7>1A2B3C4D5E6F70</hexBinary7>
+ <hexBinaryPrefixed>DEADBEEF</hexBinaryPrefixed>
Review comment:
Are there any tests with larger hex binary sizes? It seems like there's
some special cases for sizes less than 256 that might not get triggered with
only small tests?
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/HexBinaryCodeGenerator.scala
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+package org.apache.daffodil.runtime2.generators
+
+import org.apache.daffodil.dpath.NodeInfo.PrimType
+import org.apache.daffodil.grammar.primitives.HexBinaryLengthPrefixed
+import org.apache.daffodil.grammar.primitives.HexBinarySpecifiedLength
+import org.apache.daffodil.schema.annotation.props.gen.BitOrder
+import org.apache.daffodil.schema.annotation.props.gen.ByteOrder
+import org.apache.daffodil.schema.annotation.props.gen.OccursCountKind
+
+trait HexBinaryCodeGenerator extends BinaryAbstractCodeGenerator {
+
+ def hexBinaryLengthPrefixedGenerateCode(g: HexBinaryLengthPrefixed, cgState:
CodeGeneratorState): Unit = {
+ // For the time being this is a very limited back end.
+ // So there are some restrictions to enforce.
+ val e = g.e
+ e.schemaDefinitionUnless(e.bitOrder eq BitOrder.MostSignificantBitFirst,
"Only dfdl:bitOrder 'mostSignificantBitFirst' is supported.")
+
+ val fieldName = e.namedQName.local
+ val arraySize = if (e.occursCountKind == OccursCountKind.Fixed)
e.maxOccurs else 0
+ val conv = if (e.prefixedLengthElementDecl.byteOrderEv.constValue ==
ByteOrder.BigEndian) "be" else "le"
+ val intType = e.prefixedLengthElementDecl.optPrimType.get match {
+ case PrimType.Byte
+ | PrimType.Short
+ | PrimType.Int
+ | PrimType.Long
+ | PrimType.Integer => "int"
+ case PrimType.UnsignedByte
+ | PrimType.UnsignedShort
+ | PrimType.UnsignedInt
+ | PrimType.UnsignedLong
+ | PrimType.NonNegativeInteger => "uint"
+ case p => e.SDE("Prefixed length PrimType %s is not supported in C code
generator.", p.toString)
+ }
+ val intLen =
e.prefixedLengthElementDecl.elementLengthInBitsEv.constValue.get
+
+ def addStatements(deref: String): Unit = {
+ val field = s"instance->$fieldName$deref"
+ val i = if (deref.length > 2) deref.substring(1, deref.length - 1) else
""
+ val lenVar = s"_l_$fieldName$i"
+ val initStatement =
+ s""" $field.array = NULL;
+ | $field.size = 0;
+ | $field.dynamic = true;""".stripMargin
+ val parseStatement =
+ s""" $intType${intLen}_t $lenVar;
+ | parse_${conv}_$intType$intLen(&$lenVar, pstate);
+ | if (pstate->error) return;
+ | parse_alloc(&$field, $lenVar, pstate);
+ | if (pstate->error) return;
+ | parse_hexBinary(&$field, pstate);
+ | if (pstate->error) return;""".stripMargin
+ val unparseStatement =
+ s""" unparse_${conv}_$intType$intLen($field.size, ustate);
+ | if (ustate->error) return;
+ | unparse_hexBinary($field, ustate);
+ | if (ustate->error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(initStatement, parseStatement,
unparseStatement)
+
+ if (e.hasFixedValue) {
+ val variable = s"${fieldName}_fixed$i"
+ val value = e.fixedValueAsString.grouped(2).mkString(", 0x")
+ val init2 = ""
+ val parse2 =
+ s""" uint8_t $variable[] = {0x$value};
+ | parse_validate_fixed(memcmp($field.array, $variable,
sizeof($variable)) == 0, "$fieldName", pstate);
+ | if (pstate->error) return;""".stripMargin
+ val unparse2 =
+ s""" uint8_t $variable[] = {0x$value};
+ | unparse_validate_fixed(memcmp($field.array, $variable,
sizeof($variable)) == 0, "$fieldName", ustate);
+ | if (ustate->error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(init2, parse2, unparse2)
+ }
Review comment:
Might be nice to have a test to make sure this fixed stuff is covered
and works as expected.
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/parsers.c
##########
@@ -149,10 +150,47 @@ parse_fill_bytes(size_t end_position, PState *pstate)
while (pstate->position < end_position)
{
- read_stream_update_position;
+ read_stream_update_position(&buffer.c_val, 1);
}
}
+// Allocate or reallocate memory for hexBinary field
+
+void
+parse_alloc(HexBinary *hexBinary, size_t num_bytes, PState *pstate)
+{
+ // Reuse old array if possible
+ if (hexBinary->size >= num_bytes)
+ {
+ hexBinary->size = num_bytes;
+ return;
+ }
Review comment:
Note that without a capacity member, mentioned in another comment, this
will just keep making the possible array size smaller and force an allocation
if we ever need a larger hexBinary array than the previous, even if it would
fit in the capacity of the existing array. Instead, this probably wants to be
something like:
```c
// Reuse old array if possible
if (hexBinary->capacity >= num_bytes)
{
hexBinary->size = num_bytes;
return;
}
```
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/HexBinaryCodeGenerator.scala
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+package org.apache.daffodil.runtime2.generators
+
+import org.apache.daffodil.dpath.NodeInfo.PrimType
+import org.apache.daffodil.grammar.primitives.HexBinaryLengthPrefixed
+import org.apache.daffodil.grammar.primitives.HexBinarySpecifiedLength
+import org.apache.daffodil.schema.annotation.props.gen.BitOrder
+import org.apache.daffodil.schema.annotation.props.gen.ByteOrder
+import org.apache.daffodil.schema.annotation.props.gen.OccursCountKind
+
+trait HexBinaryCodeGenerator extends BinaryAbstractCodeGenerator {
+
+ def hexBinaryLengthPrefixedGenerateCode(g: HexBinaryLengthPrefixed, cgState:
CodeGeneratorState): Unit = {
+ // For the time being this is a very limited back end.
+ // So there are some restrictions to enforce.
+ val e = g.e
+ e.schemaDefinitionUnless(e.bitOrder eq BitOrder.MostSignificantBitFirst,
"Only dfdl:bitOrder 'mostSignificantBitFirst' is supported.")
+
+ val fieldName = e.namedQName.local
+ val arraySize = if (e.occursCountKind == OccursCountKind.Fixed)
e.maxOccurs else 0
+ val conv = if (e.prefixedLengthElementDecl.byteOrderEv.constValue ==
ByteOrder.BigEndian) "be" else "le"
+ val intType = e.prefixedLengthElementDecl.optPrimType.get match {
+ case PrimType.Byte
+ | PrimType.Short
+ | PrimType.Int
+ | PrimType.Long
+ | PrimType.Integer => "int"
+ case PrimType.UnsignedByte
+ | PrimType.UnsignedShort
+ | PrimType.UnsignedInt
+ | PrimType.UnsignedLong
+ | PrimType.NonNegativeInteger => "uint"
+ case p => e.SDE("Prefixed length PrimType %s is not supported in C code
generator.", p.toString)
+ }
+ val intLen =
e.prefixedLengthElementDecl.elementLengthInBitsEv.constValue.get
+
+ def addStatements(deref: String): Unit = {
+ val field = s"instance->$fieldName$deref"
+ val i = if (deref.length > 2) deref.substring(1, deref.length - 1) else
""
+ val lenVar = s"_l_$fieldName$i"
+ val initStatement =
+ s""" $field.array = NULL;
+ | $field.size = 0;
+ | $field.dynamic = true;""".stripMargin
+ val parseStatement =
+ s""" $intType${intLen}_t $lenVar;
+ | parse_${conv}_$intType$intLen(&$lenVar, pstate);
+ | if (pstate->error) return;
+ | parse_alloc(&$field, $lenVar, pstate);
+ | if (pstate->error) return;
+ | parse_hexBinary(&$field, pstate);
+ | if (pstate->error) return;""".stripMargin
+ val unparseStatement =
+ s""" unparse_${conv}_$intType$intLen($field.size, ustate);
+ | if (ustate->error) return;
+ | unparse_hexBinary($field, ustate);
+ | if (ustate->error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(initStatement, parseStatement,
unparseStatement)
+
+ if (e.hasFixedValue) {
+ val variable = s"${fieldName}_fixed$i"
+ val value = e.fixedValueAsString.grouped(2).mkString(", 0x")
+ val init2 = ""
+ val parse2 =
+ s""" uint8_t $variable[] = {0x$value};
+ | parse_validate_fixed(memcmp($field.array, $variable,
sizeof($variable)) == 0, "$fieldName", pstate);
+ | if (pstate->error) return;""".stripMargin
+ val unparse2 =
+ s""" uint8_t $variable[] = {0x$value};
+ | unparse_validate_fixed(memcmp($field.array, $variable,
sizeof($variable)) == 0, "$fieldName", ustate);
+ | if (ustate->error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(init2, parse2, unparse2)
+ }
+ }
+ if (arraySize > 0)
+ for (i <- 0 until arraySize)
+ addStatements(s"[$i]")
+ else
Review comment:
Might be worth adding an array of prefixed length hex binary to make
sure this works.
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/HexBinaryCodeGenerator.scala
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+package org.apache.daffodil.runtime2.generators
+
+import org.apache.daffodil.dpath.NodeInfo.PrimType
+import org.apache.daffodil.grammar.primitives.HexBinaryLengthPrefixed
+import org.apache.daffodil.grammar.primitives.HexBinarySpecifiedLength
+import org.apache.daffodil.schema.annotation.props.gen.BitOrder
+import org.apache.daffodil.schema.annotation.props.gen.ByteOrder
+import org.apache.daffodil.schema.annotation.props.gen.OccursCountKind
+
+trait HexBinaryCodeGenerator extends BinaryAbstractCodeGenerator {
+
+ def hexBinaryLengthPrefixedGenerateCode(g: HexBinaryLengthPrefixed, cgState:
CodeGeneratorState): Unit = {
+ // For the time being this is a very limited back end.
+ // So there are some restrictions to enforce.
+ val e = g.e
+ e.schemaDefinitionUnless(e.bitOrder eq BitOrder.MostSignificantBitFirst,
"Only dfdl:bitOrder 'mostSignificantBitFirst' is supported.")
+
+ val fieldName = e.namedQName.local
+ val arraySize = if (e.occursCountKind == OccursCountKind.Fixed)
e.maxOccurs else 0
+ val conv = if (e.prefixedLengthElementDecl.byteOrderEv.constValue ==
ByteOrder.BigEndian) "be" else "le"
+ val intType = e.prefixedLengthElementDecl.optPrimType.get match {
+ case PrimType.Byte
+ | PrimType.Short
+ | PrimType.Int
+ | PrimType.Long
+ | PrimType.Integer => "int"
+ case PrimType.UnsignedByte
+ | PrimType.UnsignedShort
+ | PrimType.UnsignedInt
+ | PrimType.UnsignedLong
+ | PrimType.NonNegativeInteger => "uint"
+ case p => e.SDE("Prefixed length PrimType %s is not supported in C code
generator.", p.toString)
+ }
+ val intLen =
e.prefixedLengthElementDecl.elementLengthInBitsEv.constValue.get
+
+ def addStatements(deref: String): Unit = {
+ val field = s"instance->$fieldName$deref"
+ val i = if (deref.length > 2) deref.substring(1, deref.length - 1) else
""
+ val lenVar = s"_l_$fieldName$i"
+ val initStatement =
+ s""" $field.array = NULL;
+ | $field.size = 0;
+ | $field.dynamic = true;""".stripMargin
+ val parseStatement =
+ s""" $intType${intLen}_t $lenVar;
+ | parse_${conv}_$intType$intLen(&$lenVar, pstate);
+ | if (pstate->error) return;
+ | parse_alloc(&$field, $lenVar, pstate);
+ | if (pstate->error) return;
+ | parse_hexBinary(&$field, pstate);
+ | if (pstate->error) return;""".stripMargin
+ val unparseStatement =
+ s""" unparse_${conv}_$intType$intLen($field.size, ustate);
+ | if (ustate->error) return;
+ | unparse_hexBinary($field, ustate);
+ | if (ustate->error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(initStatement, parseStatement,
unparseStatement)
+
+ if (e.hasFixedValue) {
+ val variable = s"${fieldName}_fixed$i"
+ val value = e.fixedValueAsString.grouped(2).mkString(", 0x")
+ val init2 = ""
+ val parse2 =
+ s""" uint8_t $variable[] = {0x$value};
+ | parse_validate_fixed(memcmp($field.array, $variable,
sizeof($variable)) == 0, "$fieldName", pstate);
+ | if (pstate->error) return;""".stripMargin
+ val unparse2 =
+ s""" uint8_t $variable[] = {0x$value};
+ | unparse_validate_fixed(memcmp($field.array, $variable,
sizeof($variable)) == 0, "$fieldName", ustate);
+ | if (ustate->error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(init2, parse2, unparse2)
+ }
+ }
+ if (arraySize > 0)
+ for (i <- 0 until arraySize)
+ addStatements(s"[$i]")
+ else
+ addStatements("")
+ }
+
+ def hexBinarySpecifiedLengthGenerateCode(g: HexBinarySpecifiedLength,
cgState: CodeGeneratorState): Unit = {
+ // For the time being this is a very limited back end.
+ // So there are some restrictions to enforce.
+ val e = g.e
+ e.schemaDefinitionUnless(e.bitOrder eq BitOrder.MostSignificantBitFirst,
"Only dfdl:bitOrder 'mostSignificantBitFirst' is supported.")
+ e.schemaDefinitionUnless(e.elementLengthInBitsEv.isConstant, "Runtime
dfdl:length expressions not supported.")
+
+ val fieldName = e.namedQName.local
+ val arraySize = if (e.occursCountKind == OccursCountKind.Fixed)
e.maxOccurs else 0
+
+ def addStatements(deref: String): Unit = {
+ val field = s"instance->$fieldName$deref"
+ val fieldArray = s"instance->_a_$fieldName$deref"
+ val initStatement =
+ s""" $field.array = $fieldArray;
+ | $field.size = sizeof($fieldArray);
+ | $field.dynamic = false;
+ | memset($fieldArray, 0x77, sizeof($fieldArray));""".stripMargin
+ val parseStatement =
+ s""" parse_hexBinary(&$field, pstate);
+ | if (pstate->error) return;""".stripMargin
+ val unparseStatement =
+ s""" unparse_hexBinary($field, ustate);
+ | if (ustate->error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(initStatement, parseStatement,
unparseStatement)
+
+ if (e.hasFixedValue) {
+ val i = if (deref.length > 2) deref.substring(1, deref.length - 1)
else ""
+ val variable = s"${fieldName}_fixed$i"
+ val value = e.fixedValueAsString.grouped(2).mkString(", 0x")
+ val init2 = ""
+ val parse2 =
+ s""" uint8_t $variable[] = {0x$value};
+ | parse_validate_fixed(memcmp($field.array, $variable,
sizeof($variable)) == 0, "$fieldName", pstate);
+ | if (pstate->error) return;""".stripMargin
+ val unparse2 =
+ s""" uint8_t $variable[] = {0x$value};
+ | unparse_validate_fixed(memcmp($field.array, $variable,
sizeof($variable)) == 0, "$fieldName", ustate);
+ | if (ustate->error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(init2, parse2, unparse2)
+ }
+ }
+ if (arraySize > 0)
+ for (i <- 0 until arraySize)
+ addStatements(s"[$i]")
+ else
+ addStatements("")
+ }
Review comment:
Seems like much of this should be the same as the prefixedLength case,
except there's extra generated code for the prefixed length? Can things be
refactor to avoid duplciate code?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
