mbeckerle commented on code in PR #1098:
URL: https://github.com/apache/daffodil/pull/1098#discussion_r1367276044
##########
BUILD.md:
##########
@@ -50,9 +50,9 @@ following its website's instructions. This is necessary to
install the
[libmxml-devel][Mini-XML] package.
You can use the `dnf` package manager to install most of the tools
-needed to build Daffodil:
+used to develop Daffodil:
- sudo dnf install clang gcc git java-11-openjdk-devel llvm make mxml-devel
pkgconf
+ sudo dnf install clang gcc git iwyu java-11-openjdk-devel llvm make
mxml-devel pkgconf
If you want to use clang instead of gcc, you'll have to set your
Review Comment:
Question: why give people the choice?
We don't have to provide this flexibility unless we or users depend on it
for some reason.
This works, so "it aint broke" so no need to change anything. I just wanted
to comment that we don't really have to provide options like this unless
they're really _really_ required.
##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/libruntime/unparsers.c:
##########
@@ -401,69 +401,41 @@ unparse_le_uint8(uint8_t number, size_t num_bits, UState
*ustate)
unparse_endian_uint64(LITTLE_ENDIAN_DATA, number, num_bits, ustate);
}
+// Unparse opaque bytes from hexBinary field
+
+void
+unparse_hexBinary(HexBinary hexBinary, UState *ustate)
+{
+ write_bits(hexBinary.array, hexBinary.lengthInBytes * BYTE_WIDTH, ustate);
+ if (ustate->pu.error) return;
+ ustate->pu.bitPos0b += hexBinary.lengthInBytes * BYTE_WIDTH;
+}
+
// Unparse fill bits up to alignmentInBits or end_bitPos0b
void
unparse_align(size_t alignmentInBits, const uint8_t fill_byte, UState *ustate)
{
- size_t end_bitPos0b = ((ustate->bitPos0b + alignmentInBits - 1) /
alignmentInBits) * alignmentInBits;
+ size_t end_bitPos0b = ((ustate->pu.bitPos0b + alignmentInBits - 1) /
alignmentInBits) * alignmentInBits;
unparse_fill_bits(end_bitPos0b, fill_byte, ustate);
}
void
unparse_fill_bits(size_t end_bitPos0b, const uint8_t fill_byte, UState *ustate)
Review Comment:
This reads better than parse_fill_bits, but I'd still prefer
unparse_alignment_bits
##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/libruntime/parsers.c:
##########
@@ -21,7 +21,7 @@
#include <stdbool.h> // for bool, false, true
#include <stdio.h> // for fread, fgetc, EOF
#include <stdlib.h> // for free, malloc
-#include "errors.h" // for Error, eof_or_error, Error::(anonymous),
ERR_LEFTOVER_DATA, add_diagnostic, get_diagnostics, ERR_ARRAY_BOUNDS,
ERR_FIXED_VALUE, ERR_HEXBINARY_ALLOC, ERR_PARSE_BOOL, Diagnostics
+#include "errors.h" // for Error, eof_or_error, ERR_LEFTOVER_DATA,
Error::(anonymous), ERR_HEXBINARY_ALLOC, ERR_PARSE_BOOL
Review Comment:
Hmm. iwyu specified what "errors.h" was needed for here, but above where SL
commented on ERR_ZZZ there's no such comment on the include of "errors.h".
What are the rules here about what iwyu maintains?
##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/tests/bits.c:
##########
Review Comment:
This file is full of unit tests. Yet it is under src/main/resources... is
this in the right place? Is this a C tooling limitation?
This wants to be excluded from delivered libs yes?
##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/tests/bits.c:
##########
@@ -20,29 +20,29 @@
#include <criterion/new/assert.h> // for CRI_ASSERT_OP_VAR_TAGGED,
CRI_ASSERT_OP_TYPE_TAGGED, CRI_ASSERT_OP_VAL_TAGGED, CRI_ASSERT_TEST_TAG_u8,
cr_user_u8_tostr, CRI_ASSERT_OP_MKNODE_TAGGED, CRI_ASSERT_OP_NAME_TAGGED,
CRI_ASSERT_TEST_TAG_sz, cr_user_sz_tostr, CRI_ASSERT_TYPE_TAG_ID_u8,
cr_user_u8_eq, CRI_ASSERT_TEST_TAG_ptr, cr_user_ptr_tostr, CRI_ASSERT_MKLIST_2,
CRI_ASSERT_SPECIFIER_OP1, CRI_ASSERT_SPECIFIER_eq, CRI_ASSERT_SPEC_OPLEN_2,
CRI_ASSERT_TEST_SPECIFIER_eq, cr_expect, CRI_ASSERT_TYPE_TAG_u8,
CRI_ASSERT_TYPE_TAG_ID_sz, cr_user_sz_eq, CRI_ASSERT_TYPE_TAG_sz,
CRI_ASSERT_TYPE_TAG_ID_ptr, cr_user_ptr_eq, CRI_ASSERT_TEST_TAGC_u8,
CRI_ASSERT_TEST_TAG_int, cr_user_int_tostr, CRI_ASSERT_TYPE_TAG_ptr,
CRI_ASSERT_TEST_TAGC_sz, CRI_ASSERT_TYPE_TAG_ID_int, cr_user_int_eq,
CRI_ASSERT_TEST_TAGC_ptr, CRI_ASSERT_TYPE_TAG_int, CRI_ASSERT_TEST_TAG_i16,
CRI_ASSERT_TEST_TAG_i32, CRI_ASSERT_TEST_TAG_i64, CRI_ASSERT_TEST_TAG_i8,
CRI_ASSERT_TEST_TAG_u16, CRI_ASSERT_TEST_TAG_u32, CRI_ASSERT_TE..
.
Review Comment:
Answers my question about iwyu making crazy long lines.
Is there a parameter so you could make this put say, 80 characters max?
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/generators/BinaryValueCodeGenerator.scala:
##########
@@ -50,6 +51,53 @@ trait BinaryValueCodeGenerator {
if (e.hasFixedValue && e.fixedValueAsString.nonEmpty) {
validateFixed(deref)
}
+
+ // Check if the element's value is restricted to a set of enumerations
+ if (e.typeDef.optRestriction.exists(_.hasEnumeration)) {
+ // Split the enumeration values into an array of strings ready to be
inserted into the C code
+ val enumerationValues =
e.typeDef.optRestriction.flatMap(_.enumerationValues).get
+ val enums = enumerationValues.split('|').map(_.replaceAll("\\\\(\\W)",
"$1"))
+ // Call another function which can be redefined differently if necessary
Review Comment:
Don't get what this comment means. Clarify.
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/generators/BinaryValueCodeGenerator.scala:
##########
@@ -50,6 +51,53 @@ trait BinaryValueCodeGenerator {
if (e.hasFixedValue && e.fixedValueAsString.nonEmpty) {
validateFixed(deref)
}
+
+ // Check if the element's value is restricted to a set of enumerations
+ if (e.typeDef.optRestriction.exists(_.hasEnumeration)) {
+ // Split the enumeration values into an array of strings ready to be
inserted into the C code
+ val enumerationValues =
e.typeDef.optRestriction.flatMap(_.enumerationValues).get
+ val enums = enumerationValues.split('|').map(_.replaceAll("\\\\(\\W)",
"$1"))
+ // Call another function which can be redefined differently if necessary
+ valueValidateEnumeration(e, deref, enums, cgState)
+ }
+
+ // Check if the element's value is restricted to a range (we will need to
handle any
+ // combination of inclusive and exclusive endpoints when generating our C
expression)
+ val hasMinExclusive = e.typeDef.optRestriction.exists(_.hasMinExclusive)
+ val hasMinInclusive = e.typeDef.optRestriction.exists(_.hasMinInclusive)
+ val hasMaxExclusive = e.typeDef.optRestriction.exists(_.hasMaxExclusive)
+ val hasMaxInclusive = e.typeDef.optRestriction.exists(_.hasMaxInclusive)
+ if (hasMinExclusive || hasMinInclusive || hasMaxExclusive ||
hasMaxInclusive) {
+ // Generate the minimum endpoint comparison
+ val minEndpoint = if (hasMinExclusive) {
+ val endpoint = e.typeDef.optRestriction.map(_.minExclusiveValue).get
+ s"""> $endpoint"""
+ } else if (hasMinInclusive) {
+ val endpoint = e.typeDef.optRestriction.map(_.minInclusiveValue).get
+ // Avoid unsigned >= 0 comparisons (causes gcc warnings)
+ e.optPrimType.get match {
+ case PrimType.UnsignedByte | PrimType.UnsignedShort |
PrimType.UnsignedInt |
+ PrimType.UnsignedLong | PrimType.NonNegativeInteger if
endpoint.toString == "0" =>
+ ""
Review Comment:
Is this stub, or really nothing to generate here? If the latter, add comment
to that effect.
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/generators/BinaryValueCodeGenerator.scala:
##########
@@ -93,11 +141,94 @@ trait BinaryValueCodeGenerator {
val initERDStatement = ""
val parseStatement =
- s"""$indent1$indent2 parse_validate_fixed($field == $fixed,
"$localName", pstate);
- |$indent1$indent2 if (pstate->error) return;""".stripMargin
+ s"""$indent1$indent2 validate_fixed_attribute($field == $fixed,
"$localName", &pstate->pu);
+ |$indent1$indent2 if (pstate->pu.error) return;""".stripMargin
+ val unparseStatement =
+ s"""$indent1$indent2 validate_fixed_attribute($field == $fixed,
"$localName", &ustate->pu);
+ |$indent1$indent2 if (ustate->pu.error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(initERDStatement, parseStatement,
unparseStatement)
+ }
+
+ // Generate C code to validate a primitive element matches one of its
enumeration values.
+ // May be replaced by a more specialized function in another trait for some
types.
Review Comment:
So why isn't there a base trait and this just gets protected not private and
another class can override in the usual OO manner?
You are accomplishing that same thing by different means, or am I completely
misunderstanding your design here?
I guess I'm expecting a enumeration validation mixin trait, which provides
the signature and overridable implementation, and people can use super to run
this and just add additional stuff too if they want.
Or maybe that can't work for some reason.
In other words, it feels like there's maybe some OO refactoring stuff that
could happen here. But it's not required in this change set. What you have here
is pretty nice anyway. Just a suggestion for future thought.
##########
daffodil-codegen-c/src/test/resources/org/apache/daffodil/codegen/c/ex_nums.dfdl.xsd:
##########
@@ -17,184 +17,197 @@
-->
<xs:schema
- elementFormDefault="qualified"
- targetNamespace="http://example.com";
- xmlns="http://example.com";
- xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/";
- xmlns:xs="http://www.w3.org/2001/XMLSchema";>
+ elementFormDefault="qualified"
+ targetNamespace="http://example.com";
+ version="1.2.3"
+ xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/";
+ xmlns:net="urn:network/format"
+ xmlns:xs="http://www.w3.org/2001/XMLSchema";
+ xmlns="http://example.com";>
- <!-- Representation property bindings -->
+ <!-- Network order binary format (net:format) -->
Review Comment:
I know folks at Sun (back in the day) and IETF wanted to define "Network
Byte Order", but they failed because Intel little-endian CPUs took over the
world for a while and everybody serialized data little endian because it was
easiest. Nobody sees the TCP/IP headers of packets. (or cares) They see their
payload data, which is little-endian a huge fraction of the time.
So please say big or little endian here.
##########
daffodil-codegen-c/src/test/resources/org/apache/daffodil/codegen/c/network/format.dfdl.xsd:
##########
@@ -0,0 +1,50 @@
+<?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.
+-->
+
+<!-- Network order binary format:
<https://en.wikipedia.org/wiki/Endianness#Networking> -->
+
+<schema
+ targetNamespace="urn:network/format"
+ xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/";
+ xmlns:net="urn:network/format"
+ xmlns:xs="http://www.w3.org/2001/XMLSchema";
+ xmlns="http://www.w3.org/2001/XMLSchema";>
+
+ <!-- Network order binary format (net:format) -->
+
+ <include
schemaLocation="/org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <annotation>
+ <appinfo source="http://www.ogf.org/dfdl/";>
+ <dfdl:defineFormat name="format">
+ <dfdl:format
Review Comment:
You really should define byteOrder here as bigEndian and bitOrder as
mostSignificantBitFirst, because it matters in order to understand this format.
DFDLGeneralFormat.dfdl.xsd is not supposed to change, but it's also supposed
to be mostly so you don't have to remember obscure properties Daffodil requires
that your format otherwise wouldn't bother to mention.
For properties that really define the essence of the format, you should have
them explicitly regardless of whether they match what is in DFDLGeneralFormat
or not.
##########
daffodil-codegen-c/src/test/resources/org/apache/daffodil/codegen/c/simple.dfdl.xsd:
##########
@@ -18,45 +18,351 @@
<schema
targetNamespace="urn:simple"
+ version="2.1.1"
xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/";
+ xmlns:net="urn:network/format"
xmlns:si="urn:simple"
xmlns:xs="http://www.w3.org/2001/XMLSchema";
xmlns="http://www.w3.org/2001/XMLSchema";>
- <!-- Binary representation properties -->
+ <!-- Network order binary format (net:format) -->
- <include
schemaLocation="/org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <import namespace="urn:network/format"
schemaLocation="network/format.dfdl.xsd"/>
<annotation>
<appinfo source="http://www.ogf.org/dfdl/";>
- <dfdl:format
- binaryBooleanFalseRep="0"
- binaryBooleanTrueRep="1"
- fillByte="%NUL;"
- prefixIncludesPrefixLength="no"
- ref="si:GeneralFormat"
- representation="binary"/>
+ <dfdl:format ref="net:format"/>
</appinfo>
</annotation>
- <!-- Root elements (pick only one) -->
-
- <element name="simple-boolean" type="xs:boolean"/>
- <element name="simple-byte" type="xs:byte"/>
- <element name="simple-double" type="xs:double"/>
- <element name="simple-float" type="xs:float"/>
- <element name="simple-hexBinary" type="xs:hexBinary" dfdl:length="4"
dfdl:lengthKind="explicit" dfdl:lengthUnits="bytes"/>
- <element name="simple-hexBinaryPrefixed" type="xs:hexBinary"
dfdl:lengthKind="prefixed" dfdl:lengthUnits="bytes"
dfdl:prefixLengthType="si:prefixedCount"/>
- <element name="simple-int" type="xs:int"/>
- <element name="simple-integer" type="xs:integer" dfdl:length="4"
dfdl:lengthKind="explicit" dfdl:lengthUnits="bytes"/>
- <element name="simple-long" type="xs:long"/>
- <element name="simple-nonNegativeInteger" type="xs:nonNegativeInteger"
dfdl:length="4" dfdl:lengthKind="explicit" dfdl:lengthUnits="bytes"/>
- <element name="simple-short" type="xs:short"/>
- <element name="simple-unsignedByte" type="xs:unsignedByte"/>
- <element name="simple-unsignedInt" type="xs:unsignedInt"/>
- <element name="simple-unsignedLong" type="xs:unsignedLong"/>
- <element name="simple-unsignedShort" type="xs:unsignedShort"/>
-
- <!-- Simple data types -->
+ <!-- Simple type root elements (pick only one) -->
+
+ <element name="simple-boolean" type="si:simple-boolean"/>
+ <element name="simple-byte" type="si:simple-byte"/>
+ <element name="simple-double" type="si:simple-double"/>
+ <element name="simple-float" type="si:simple-float"/>
+ <element name="simple-hexBinary" type="si:simple-hexBinary"/>
+ <element name="simple-hexBinaryPrefixed" type="si:simple-hexBinaryPrefixed"/>
+ <element name="simple-int" type="si:simple-int"/>
+ <element name="simple-integer" type="si:simple-integer"/>
+ <element name="simple-long" type="si:simple-long"/>
+ <element name="simple-nonNegativeInteger"
type="si:simple-nonNegativeInteger"/>
+ <element name="simple-short" type="si:simple-short"/>
+ <element name="simple-unsignedByte" type="si:simple-unsignedByte"/>
+ <element name="simple-unsignedInt" type="si:simple-unsignedInt"/>
+ <element name="simple-unsignedLong" type="si:simple-unsignedLong"/>
+ <element name="simple-unsignedShort" type="si:simple-unsignedShort"/>
+
+ <!-- Enumerations of simple type root elements (pick only one) -->
+
+ <!-- DFDL specification does not allow enumerations of boolean types -->
+ <element name="enum-byte" type="si:enum-byte"/>
+ <element name="enum-double" type="si:enum-double"/>
+ <element name="enum-float" type="si:enum-float"/>
+ <element name="enum-hexBinary" type="si:enum-hexBinary"/>
+ <element name="enum-hexBinaryPrefixed" type="si:enum-hexBinaryPrefixed"/>
+ <element name="enum-int" type="si:enum-int"/>
+ <element name="enum-integer" type="si:enum-integer"/>
+ <element name="enum-long" type="si:enum-long"/>
+ <element name="enum-nonNegativeInteger" type="si:enum-nonNegativeInteger"/>
+ <element name="enum-short" type="si:enum-short"/>
+ <element name="enum-unsignedByte" type="si:enum-unsignedByte"/>
+ <element name="enum-unsignedInt" type="si:enum-unsignedInt"/>
+ <element name="enum-unsignedLong" type="si:enum-unsignedLong"/>
+ <element name="enum-unsignedShort" type="si:enum-unsignedShort"/>
+
+ <!-- Ranges of simple type root elements (pick only one) -->
+
+ <!-- DFDL specification does not allow ranges of boolean types -->
+ <element name="range-byte" type="si:range-byte"/>
+ <element name="range-double" type="si:range-double"/>
+ <element name="range-float" type="si:range-float"/>
+ <!-- DFDL specification does not allow ranges of hexBinary types -->
+ <element name="range-int" type="si:range-int"/>
+ <element name="range-integer" type="si:range-integer"/>
+ <element name="range-long" type="si:range-long"/>
+ <element name="range-nonNegativeInteger" type="si:range-nonNegativeInteger"/>
+ <element name="range-short" type="si:range-short"/>
+ <element name="range-unsignedByte" type="si:range-unsignedByte"/>
+ <element name="range-unsignedInt" type="si:range-unsignedInt"/>
+ <element name="range-unsignedLong" type="si:range-unsignedLong"/>
+ <element name="range-unsignedShort" type="si:range-unsignedShort"/>
+
+ <!-- All-in-one root element (pick when you want everything) -->
+
+ <element name="simple">
+ <xs:complexType>
+ <sequence>
+ <!-- All simple types -->
+ <element name="simple-boolean" type="si:simple-boolean"/>
+ <element name="simple-byte" type="si:simple-byte"/>
+ <element name="simple-double" type="si:simple-double"/>
+ <element name="simple-float" type="si:simple-float"/>
+ <element name="simple-hexBinary" type="si:simple-hexBinary"/>
+ <element name="simple-hexBinaryPrefixed"
type="si:simple-hexBinaryPrefixed"/>
+ <element name="simple-int" type="si:simple-int"/>
+ <element name="simple-integer" type="si:simple-integer"/>
+ <element name="simple-long" type="si:simple-long"/>
+ <element name="simple-nonNegativeInteger"
type="si:simple-nonNegativeInteger"/>
+ <element name="simple-short" type="si:simple-short"/>
+ <element name="simple-unsignedByte" type="si:simple-unsignedByte"/>
+ <element name="simple-unsignedInt" type="si:simple-unsignedInt"/>
+ <element name="simple-unsignedLong" type="si:simple-unsignedLong"/>
+ <element name="simple-unsignedShort" type="si:simple-unsignedShort"/>
+ <!-- All enumerations of simple types -->
+ <element name="enum-byte" type="si:enum-byte"/>
+ <element name="enum-double" type="si:enum-double"/>
+ <element name="enum-float" type="si:enum-float"/>
+ <element name="enum-hexBinary" type="si:enum-hexBinary"/>
+ <element name="enum-hexBinaryPrefixed"
type="si:enum-hexBinaryPrefixed"/>
+ <element name="enum-int" type="si:enum-int"/>
+ <element name="enum-integer" type="si:enum-integer"/>
+ <element name="enum-long" type="si:enum-long"/>
+ <element name="enum-nonNegativeInteger"
type="si:enum-nonNegativeInteger"/>
+ <element name="enum-short" type="si:enum-short"/>
+ <element name="enum-unsignedByte" type="si:enum-unsignedByte"/>
+ <element name="enum-unsignedInt" type="si:enum-unsignedInt"/>
+ <element name="enum-unsignedLong" type="si:enum-unsignedLong"/>
+ <element name="enum-unsignedShort" type="si:enum-unsignedShort"/>
+ <!-- All ranges of simple types -->
+ <element name="range-byte" type="si:range-byte"/>
+ <element name="range-double" type="si:range-double"/>
+ <element name="range-float" type="si:range-float"/>
+ <element name="range-int" type="si:range-int"/>
+ <element name="range-integer" type="si:range-integer"/>
+ <element name="range-long" type="si:range-long"/>
+ <element name="range-nonNegativeInteger"
type="si:range-nonNegativeInteger"/>
+ <element name="range-short" type="si:range-short"/>
+ <element name="range-unsignedByte" type="si:range-unsignedByte"/>
+ <element name="range-unsignedInt" type="si:range-unsignedInt"/>
+ <element name="range-unsignedLong" type="si:range-unsignedLong"/>
+ <element name="range-unsignedShort" type="si:range-unsignedShort"/>
+ </sequence>
+ </xs:complexType>
+ </element>
+
+ <!-- Simple types without any schema facets -->
+
+ <simpleType name="simple-boolean">
+ <restriction base="xs:boolean"/>
+ </simpleType>
+ <simpleType name="simple-byte">
+ <restriction base="xs:byte"/>
+ </simpleType>
+ <simpleType name="simple-double">
+ <restriction base="xs:double"/>
+ </simpleType>
+ <simpleType name="simple-float">
+ <restriction base="xs:float"/>
+ </simpleType>
+ <simpleType name="simple-hexBinary"
+ dfdl:length="4"
+ dfdl:lengthKind="explicit"
+ dfdl:lengthUnits="bytes">
+ <restriction base="xs:hexBinary"/>
+ </simpleType>
+ <simpleType name="simple-hexBinaryPrefixed"
+ dfdl:prefixLengthType="si:prefixedCount"
+ dfdl:lengthKind="prefixed"
Review Comment:
BTW: there was a DFDL clarification recently that a prefixedLengthType can
have facets, and those are checked so as to be able to have say, a 32-bit
integer as a lengthKind, but specify only sane values like < 1Million and even
minimum lengths are to be checked.
This is super necessary if reading from a TCP socket. Last thing you want to
do is a blocking read for 4Gigs of data.
Is generated C code going to implement this prefix value checking with your
range validators?
##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/libcli/daffodil_main.c:
##########
@@ -21,8 +21,8 @@
#include <string.h> // for strcmp
#include "cli_errors.h" // for CLI_DIAGNOSTICS, CLI_FILE_CLOSE,
CLI_FILE_OPEN
#include "daffodil_getopt.h" // for daffodil_cli, daffodil_parse,
daffodil_parse_cli, parse_daffodil_cli, daffodil_unparse, daffodil_unparse_cli,
DAFFODIL_PARSE, DAFFODIL_UNPARSE
-#include "errors.h" // for continue_or_exit, print_diagnostics,
Error, Diagnostics, Error::(anonymous)
-#include "infoset.h" // for get_infoset, walk_infoset, PState,
UState, parse_data, unparse_infoset, InfosetBase, VisitEventHandler
+#include "errors.h" // for continue_or_exit, Diagnostics,
print_diagnostics, Error
+#include "infoset.h" // for ParserOrUnparserState, PState, UState,
get_infoset, walk_infoset, parse_data, unparse_infoset, InfosetBase,
VisitEventHandler
Review Comment:
Maybe just add a comment above the auto-maintained part indicating that
these comments and the include list are auto-maintained by iwyu. I know the
whole concept of this was news to me when I first read this code.
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/generators/BinaryValueCodeGenerator.scala:
##########
@@ -93,11 +141,94 @@ trait BinaryValueCodeGenerator {
val initERDStatement = ""
val parseStatement =
- s"""$indent1$indent2 parse_validate_fixed($field == $fixed,
"$localName", pstate);
- |$indent1$indent2 if (pstate->error) return;""".stripMargin
+ s"""$indent1$indent2 validate_fixed_attribute($field == $fixed,
"$localName", &pstate->pu);
+ |$indent1$indent2 if (pstate->pu.error) return;""".stripMargin
+ val unparseStatement =
+ s"""$indent1$indent2 validate_fixed_attribute($field == $fixed,
"$localName", &ustate->pu);
+ |$indent1$indent2 if (ustate->pu.error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(initERDStatement, parseStatement,
unparseStatement)
+ }
+
+ // Generate C code to validate a primitive element matches one of its
enumeration values.
+ // May be replaced by a more specialized function in another trait for some
types.
+ private def valueValidateEnumeration(
+ e: ElementBase,
+ deref: String,
+ enums: Array[String],
+ cgState: CodeGeneratorState,
+ ): Unit = {
+ val indent1 = if (cgState.hasChoice) INDENT else NO_INDENT
+ val indent2 = if (deref.nonEmpty) INDENT else NO_INDENT
+ val localName = cgState.cName(e)
+ val field = s"instance->$localName$deref"
+
+ // Extra initialization only for hexBinary enumerations
+ val arraysName = s"arrays_$localName"
+ val hexEnums = enums.zipWithIndex.map { case (s, index) =>
+ s"{$arraysName[$index], ${s.length / 2}, false}"
+ }
+ val hexEnumsInit = enums.map(_.grouped(2).map("0x" + _).mkString("{", ",
", "}"))
+ val enumsLenMax = enums.map(_.length / 2).max
+ val arraysInit =
+ s"""$indent1$indent2 uint8_t $arraysName[][$enumsLenMax] =
${hexEnumsInit.mkString(
+ "{",
+ ", ",
+ "}",
+ )};\n"""
Review Comment:
I have never seen enumeration facets on hexBinary, so I wouldn't sweat it. I
had to check the DFDL spec to see if they were even allowed!
##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/libruntime/infoset.h:
##########
@@ -102,52 +102,56 @@ typedef struct ERD
typedef struct HexBinary
{
- uint8_t *array; // pointer to data in byte array
- size_t lengthInBytes; // length of data in bytes
- bool dynamic; // true if byte array was malloc'ed
+ uint8_t *array; // pointer to data in byte array
+ size_t lengthInBytes; // length of data in bytes
+ bool dynamic; // true if byte array was malloc'ed
} HexBinary;
// InfosetBase - metadata of an infoset element
typedef struct InfosetBase
{
- const ERD * erd;
+ const ERD *erd;
const struct InfosetBase *parent;
} InfosetBase;
+// ParserOrUnparserState - common mutable state while parsing or unparsing
+
+typedef struct ParserOrUnparserState
+{
+ FILE *stream; // stream to read from / write to
+ size_t bitPos0b; // 0-based position after last read/write (1-bit
granularity)
+ Diagnostics *diagnostics; // any validation diagnostics
+ const Error *error; // any error which stops parser/unparser
+} ParserOrUnparserState;
+
// PState - mutable state while parsing data
typedef struct PState
{
- FILE * stream; // stream to read data from (input)
- size_t bitPos0b; // 0-based position of last successful parse
(1-bit granularity)
- Diagnostics *diagnostics; // any validation diagnostics
- const Error *error; // any error which stops parse
- uint8_t unreadBits; // any buffered bits not read yet
- uint8_t numUnreadBits; // number of buffered bits not read yet
+ ParserOrUnparserState pu; // common mutable state
Review Comment:
Yessssss. embedded by value, not by reference. :+1:
##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/libruntime/parsers.c:
##########
@@ -563,36 +535,36 @@ void
parse_hexBinary(HexBinary *hexBinary, PState *pstate)
{
read_bits(hexBinary->array, hexBinary->lengthInBytes * BYTE_WIDTH, pstate);
- if (pstate->error) return;
- pstate->bitPos0b += hexBinary->lengthInBytes * BYTE_WIDTH;
+ if (pstate->pu.error) return;
+ pstate->pu.bitPos0b += hexBinary->lengthInBytes * BYTE_WIDTH;
}
-// Validate element's value matches its fixed attribute
+// Parse fill bits up to alignmentInBits or end_bitPos0b
void
-parse_validate_fixed(bool same, const char *element, PState *pstate)
+parse_align(size_t alignmentInBits, PState *pstate)
{
- if (!same)
- {
- Diagnostics *diagnostics = get_diagnostics();
- const Error error = {ERR_FIXED_VALUE, {.s = element}};
-
- add_diagnostic(diagnostics, &error);
- pstate->diagnostics = diagnostics;
- }
+ size_t end_bitPos0b = ((pstate->pu.bitPos0b + alignmentInBits - 1) /
alignmentInBits) * alignmentInBits;
+ parse_fill_bits(end_bitPos0b, pstate);
}
-// Check array count is within bounds
-
void
-parse_check_bounds(const char *name, size_t count, size_t minOccurs, size_t
maxOccurs, PState *pstate)
+parse_fill_bits(size_t end_bitPos0b, PState *pstate)
Review Comment:
This name confused me. It has to be "fill" as in short for the alignmentFill
region.
Do you mind changing the name to parse_alignment_bits?
##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/tests/bits.c:
##########
@@ -125,25 +125,25 @@ Test(bits, be_bool_4)
Test(bits, be_bool_7_7)
{
// Open memory stream for writing to dynamic buffer
- char * buffer = NULL;
- size_t size = 0;
- FILE * stream = open_memstream(&buffer, &size);
- UState ustate = {stream, 0, NULL, NULL, 0, 0};
+ char *buffer = NULL;
+ size_t size = 0;
+ FILE *stream = open_memstream(&buffer, &size);
+ UState ustate = {{stream, 0, NULL, NULL}, 0, 0};
uint32_t true7_rep = 0146; // 0b_1_100_110
uint32_t false_rep = 0157; // 0b_1_101_111
// Verify that ustate writes 11001101
unparse_be_bool(true, 7, true7_rep, false_rep, &ustate);
fflush(stream);
- cr_expect(eq(ptr, (void *)ustate.error, 0), "ustate should have no error");
- cr_expect(eq(sz, ustate.bitPos0b, 7), "ustate should advance 7 bits");
+ cr_expect(eq(ptr, (void *)ustate.pu.error, 0), "ustate should have no
error");
Review Comment:
Really excellent tests.
##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/libruntime/infoset.h:
##########
@@ -102,52 +102,56 @@ typedef struct ERD
typedef struct HexBinary
{
- uint8_t *array; // pointer to data in byte array
- size_t lengthInBytes; // length of data in bytes
- bool dynamic; // true if byte array was malloc'ed
+ uint8_t *array; // pointer to data in byte array
+ size_t lengthInBytes; // length of data in bytes
+ bool dynamic; // true if byte array was malloc'ed
} HexBinary;
// InfosetBase - metadata of an infoset element
typedef struct InfosetBase
{
- const ERD * erd;
+ const ERD *erd;
const struct InfosetBase *parent;
} InfosetBase;
+// ParserOrUnparserState - common mutable state while parsing or unparsing
Review Comment:
Add to this comment that this enables some validation operations that work
whether parsing or unparsing.
(Otherwise it just seems like a painful addition requiring you to remember
state->pu.field instead of just state->field everywhere you use a common field.)
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/generators/AlignmentFillCodeGenerator.scala:
##########
@@ -31,11 +31,11 @@ trait AlignmentFillCodeGenerator {
val parseStatement =
s"""$indent1$indent2 // Fill to closest alignment
|$indent1$indent2 parse_align($alignmentInBits, pstate);
- |$indent1$indent2 if (pstate->error) return;""".stripMargin
+ |$indent1$indent2 if (pstate->pu.error) return;""".stripMargin
val unparseStatement =
s"""$indent1$indent2 // Fill to closest alignment
Review Comment:
There really should be a string whatever they call this which is
`ssm"""....multiline stuff..."""` where ssm is string-strip-margin. Since that
is such a common idiom for generators.
##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/libruntime/validators.c:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.
+ */
+
+// clang-format off
+#include "validators.h"
+#include <stdbool.h> // for bool, false, true
+#include <string.h> // for memcmp
+#include "errors.h" // for add_diagnostic, get_diagnostics, Error,
Diagnostics, ERR_ENUM_MATCH, ERR_ARRAY_BOUNDS, ERR_FIXED_VALUE,
ERR_OUTSIDE_RANGE, Error::(anonymous)
+// clang-format on
+
+// Validate element's array count is within its array bounds
+
+void
+validate_array_bounds(const char *name, size_t count, size_t minOccurs, size_t
maxOccurs,
+ ParserOrUnparserState *pustate)
+{
+ if (count < minOccurs || count > maxOccurs)
+ {
+ // Array count is not within bounds, so report error
+ static Error error = {ERR_ARRAY_BOUNDS, {0}};
+ error.arg.s = name;
+ pustate->error = &error;
+ }
+}
+
+// Validate element's value is same as its fixed attribute
+
+void
+validate_fixed_attribute(bool same, const char *element, ParserOrUnparserState
*pustate)
+{
+ if (!same)
+ {
+ // Element is not same as its fixed attribute, so diagnose problem
+ Diagnostics *diagnostics = get_diagnostics();
+ const Error error = {ERR_FIXED_VALUE, {.s = element}};
+
+ add_diagnostic(diagnostics, &error);
+ pustate->diagnostics = diagnostics;
+ }
+}
+
+// Validate element's value matches an enumeration (either floating point or
integer)
+
+void
+validate_floatpt_enumeration(double number, size_t num_enums, double enums[],
const char *element,
+ ParserOrUnparserState *pustate)
+{
+ bool match_found = false;
+ for (size_t i = 0; !match_found && i < num_enums; i++)
+ {
+ if (number == enums[i])
+ {
+ match_found = true;
+ }
+ }
+ if (!match_found)
+ {
+ // Number does not match any enumeration, so diagnose problem
+ Diagnostics *diagnostics = get_diagnostics();
+ const Error error = {ERR_ENUM_MATCH, {.s = element}};
+
+ add_diagnostic(diagnostics, &error);
+ pustate->diagnostics = diagnostics;
+ }
+}
+
+void
+validate_hexbinary_enumeration(const HexBinary *hexBinary, size_t num_enums,
HexBinary enums[],
Review Comment:
Might add a comment that this compares the bytes, not the hex digit
characters.
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/generators/BinaryValueCodeGenerator.scala:
##########
@@ -93,11 +141,94 @@ trait BinaryValueCodeGenerator {
val initERDStatement = ""
val parseStatement =
- s"""$indent1$indent2 parse_validate_fixed($field == $fixed,
"$localName", pstate);
- |$indent1$indent2 if (pstate->error) return;""".stripMargin
+ s"""$indent1$indent2 validate_fixed_attribute($field == $fixed,
"$localName", &pstate->pu);
+ |$indent1$indent2 if (pstate->pu.error) return;""".stripMargin
+ val unparseStatement =
+ s"""$indent1$indent2 validate_fixed_attribute($field == $fixed,
"$localName", &ustate->pu);
+ |$indent1$indent2 if (ustate->pu.error) return;""".stripMargin
+ cgState.addSimpleTypeStatements(initERDStatement, parseStatement,
unparseStatement)
+ }
+
+ // Generate C code to validate a primitive element matches one of its
enumeration values.
+ // May be replaced by a more specialized function in another trait for some
types.
+ private def valueValidateEnumeration(
+ e: ElementBase,
+ deref: String,
+ enums: Array[String],
+ cgState: CodeGeneratorState,
+ ): Unit = {
+ val indent1 = if (cgState.hasChoice) INDENT else NO_INDENT
+ val indent2 = if (deref.nonEmpty) INDENT else NO_INDENT
+ val localName = cgState.cName(e)
+ val field = s"instance->$localName$deref"
+
+ // Extra initialization only for hexBinary enumerations
+ val arraysName = s"arrays_$localName"
+ val hexEnums = enums.zipWithIndex.map { case (s, index) =>
+ s"{$arraysName[$index], ${s.length / 2}, false}"
+ }
+ val hexEnumsInit = enums.map(_.grouped(2).map("0x" + _).mkString("{", ",
", "}"))
+ val enumsLenMax = enums.map(_.length / 2).max
+ val arraysInit =
+ s"""$indent1$indent2 uint8_t $arraysName[][$enumsLenMax] =
${hexEnumsInit.mkString(
+ "{",
+ ", ",
+ "}",
+ )};\n"""
+
+ val (enumsInit, extraInit, primType, valType, fieldArg) =
e.optPrimType.get match {
+ case PrimType.Double | PrimType.Float => (enums, "", "double",
"floatpt", field)
+ case PrimType.HexBinary => (hexEnums, arraysInit, "HexBinary",
"hexbinary", s"&$field")
+ case _ => (enums, "", "int64_t", "integer", field)
+ }
+
+ // Initialization for all enumerations
+ val enumsArray = s"enums_$localName"
+ val varsInit = extraInit +
+ s"""$indent1$indent2 $primType $enumsArray[] = ${enumsInit.mkString(
Review Comment:
Wow, mkString takes 3 arguments? I did not know....
Feels like this could generate very long lines if there are big enums.
We have DFDL schemas with enums with 4096 entries. (That's the biggest. 12
bit integer enum, part of Link16). We have others with hundreds of enum values.
(Our schema generators have switched to putting each enum in a separate
schema file because scrolling around with these things in an edit buffer is
painful.)
Will giant enums blow up the C compiler with too long lines?
##########
daffodil-codegen-c/src/test/resources/org/apache/daffodil/codegen/c/network/format.dfdl.xsd:
##########
@@ -0,0 +1,50 @@
+<?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.
+-->
+
+<!-- Network order binary format:
<https://en.wikipedia.org/wiki/Endianness#Networking> -->
Review Comment:
Really. Just tell people this is big endian.
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/generators/BinaryValueCodeGenerator.scala:
##########
@@ -50,6 +51,53 @@ trait BinaryValueCodeGenerator {
if (e.hasFixedValue && e.fixedValueAsString.nonEmpty) {
validateFixed(deref)
}
+
+ // Check if the element's value is restricted to a set of enumerations
+ if (e.typeDef.optRestriction.exists(_.hasEnumeration)) {
+ // Split the enumeration values into an array of strings ready to be
inserted into the C code
+ val enumerationValues =
e.typeDef.optRestriction.flatMap(_.enumerationValues).get
+ val enums = enumerationValues.split('|').map(_.replaceAll("\\\\(\\W)",
"$1"))
+ // Call another function which can be redefined differently if necessary
+ valueValidateEnumeration(e, deref, enums, cgState)
+ }
+
+ // Check if the element's value is restricted to a range (we will need to
handle any
+ // combination of inclusive and exclusive endpoints when generating our C
expression)
+ val hasMinExclusive = e.typeDef.optRestriction.exists(_.hasMinExclusive)
+ val hasMinInclusive = e.typeDef.optRestriction.exists(_.hasMinInclusive)
+ val hasMaxExclusive = e.typeDef.optRestriction.exists(_.hasMaxExclusive)
+ val hasMaxInclusive = e.typeDef.optRestriction.exists(_.hasMaxInclusive)
+ if (hasMinExclusive || hasMinInclusive || hasMaxExclusive ||
hasMaxInclusive) {
+ // Generate the minimum endpoint comparison
+ val minEndpoint = if (hasMinExclusive) {
+ val endpoint = e.typeDef.optRestriction.map(_.minExclusiveValue).get
+ s"""> $endpoint"""
+ } else if (hasMinInclusive) {
+ val endpoint = e.typeDef.optRestriction.map(_.minInclusiveValue).get
+ // Avoid unsigned >= 0 comparisons (causes gcc warnings)
+ e.optPrimType.get match {
+ case PrimType.UnsignedByte | PrimType.UnsignedShort |
PrimType.UnsignedInt |
+ PrimType.UnsignedLong | PrimType.NonNegativeInteger if
endpoint.toString == "0" =>
+ ""
+ case _ =>
+ s""">= $endpoint"""
+ }
+ } else {
+ ""
+ }
+ // Generate the maximum endpoint comparison
+ val maxEndpoint = if (hasMaxExclusive) {
+ val endpoint = e.typeDef.optRestriction.map(_.maxExclusiveValue).get
+ s"""< $endpoint"""
+ } else if (hasMaxInclusive) {
+ val endpoint = e.typeDef.optRestriction.map(_.maxInclusiveValue).get
+ s"""<= $endpoint"""
+ } else {
+ ""
+ }
+ // Call another function which can be redefined differently if necessary
Review Comment:
Yeah, same comment. I don't get your drift here. Clarify. Like of course the
next thing is to call another thing. What do you mean by "can be redefined if
necessary", what would change here in that case? Are you saying you might not
want to call valueValidateRange? or that you might want to change the
behavior/checking in valueValidateRange? Everything I just said goes without
saying, so maybe just delete the comment?
##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/libruntime/validators.c:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.
+ */
+
+// clang-format off
+#include "validators.h"
+#include <stdbool.h> // for bool, false, true
+#include <string.h> // for memcmp
+#include "errors.h" // for add_diagnostic, get_diagnostics, Error,
Diagnostics, ERR_ENUM_MATCH, ERR_ARRAY_BOUNDS, ERR_FIXED_VALUE,
ERR_OUTSIDE_RANGE, Error::(anonymous)
+// clang-format on
+
+// Validate element's array count is within its array bounds
+
+void
+validate_array_bounds(const char *name, size_t count, size_t minOccurs, size_t
maxOccurs,
+ ParserOrUnparserState *pustate)
+{
+ if (count < minOccurs || count > maxOccurs)
+ {
+ // Array count is not within bounds, so report error
+ static Error error = {ERR_ARRAY_BOUNDS, {0}};
+ error.arg.s = name;
+ pustate->error = &error;
+ }
+}
+
+// Validate element's value is same as its fixed attribute
+
+void
+validate_fixed_attribute(bool same, const char *element, ParserOrUnparserState
*pustate)
+{
+ if (!same)
+ {
+ // Element is not same as its fixed attribute, so diagnose problem
+ Diagnostics *diagnostics = get_diagnostics();
+ const Error error = {ERR_FIXED_VALUE, {.s = element}};
+
+ add_diagnostic(diagnostics, &error);
+ pustate->diagnostics = diagnostics;
+ }
+}
+
+// Validate element's value matches an enumeration (either floating point or
integer)
Review Comment:
I have never ever seen a XSD with a floating point type and enumerations.
Ditto for decimal.
A specialized validator for the integer case I can see. But floating point?
What's the point (heh)?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaDocument.scala:
##########
@@ -212,6 +212,8 @@ final class SchemaDocument private (xmlSDoc:
XMLSchemaDocument)
final override lazy val optLexicalParent = Some(xmlSDoc)
final override lazy val optXMLSchemaDocument = Some(xmlSDoc)
+ final lazy val version = (xml \ "@version").text
Review Comment:
Somewhere on WWW is a diatribe on why using namespace URIs with versions in
them is so wrong wrong wrong as a versioning means for XSD schemas.
##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/libruntime/validators.c:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.
+ */
+
+// clang-format off
+#include "validators.h"
+#include <stdbool.h> // for bool, false, true
+#include <string.h> // for memcmp
+#include "errors.h" // for add_diagnostic, get_diagnostics, Error,
Diagnostics, ERR_ENUM_MATCH, ERR_ARRAY_BOUNDS, ERR_FIXED_VALUE,
ERR_OUTSIDE_RANGE, Error::(anonymous)
+// clang-format on
+
+// Validate element's array count is within its array bounds
+
+void
+validate_array_bounds(const char *name, size_t count, size_t minOccurs, size_t
maxOccurs,
+ ParserOrUnparserState *pustate)
Review Comment:
+1 for consistently calling these "pu", which is not too short if it's
consistent. In fact I almost suggested calling it c for "common" part of the
state, which would be just 1 letter.
These validation functions are the only thing that I find here that actually
uses states polymorphically via ParserOrUnparserState as a type. Is that
correct?
--
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]
