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/02/09 15:58:31 UTC
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #488: Support choices in runtime2 code generator with restrictions
stevedlawrence commented on a change in pull request #488:
URL: https://github.com/apache/incubator-daffodil/pull/488#discussion_r572977604
##########
File path: daffodil-runtime2/src/main/resources/examples/ex_nums.c
##########
@@ -660,10 +559,10 @@ littleEndian_unparseSelf(const littleEndian *instance, UState *ustate)
static void
ex_nums_initSelf(ex_nums *instance)
{
+ instance->_base.erd = &ex_nums_ERD;
array_initSelf(&instance->array);
bigEndian_initSelf(&instance->bigEndian);
littleEndian_initSelf(&instance->littleEndian);
- instance->_base.erd = &ex_nums__ERD;
}
Review comment:
What are these src/main/resources/examples files for? Are these examples of what the code generate will create? Are these for testing purposes? Perhaps they should be in src/test/resources?
##########
File path: daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala
##########
@@ -105,41 +129,178 @@ class CodeGeneratorState {
qnameInit
}
+ /**
+ * We want to convert a choiceDispatchKey expression into C struct dot
+ * notation (rootElement->[subElement.field]) which will access the C
+ * struct field containing the choiceDispatchKey's runtime value.
+ *
+ * We make some assumptions to make generating the dot notation easier:
+ * - the expression starts with '{xs:string( and ends with )}'
+ * - the expression returns the value of a previous element without
+ * changing the value in any way (except converting it to xs:string)
+ * - both the expression and the C code use only local names (for now...)
+ * - we can map the context node's path to a Unix-like slash path
+ * - all dpath operations look like Unix-like relative paths (../tag)
+ * - we can normalize the new path and convert it to C struct dot notation
+ * - we can store the accessed value in an int64_t local variable safely
+ */
+ private def choiceDispatchField(context: ElementBase): String = {
+ // We want to use SchemaComponent.scPath but it's private so duplicate it here (for now...)
+ def scPath(sc: SchemaComponent): Seq[SchemaComponent] = sc.optLexicalParent.map { scPath }.getOrElse(Nil) :+ sc
+ val localNames = scPath(context).map {
+ case er: AbstractElementRef => er.refQName.local
+ case e: ElementBase => e.namedQName.local
+ case ed: GlobalElementDecl => ed.namedQName.local
+ case _ => ""
+ }
+ val absoluteSlashPath = localNames.mkString("/")
+ val dispatchSlashPath = context.complexType.modelGroup match {
+ case choice: Choice if choice.isDirectDispatch =>
+ val expr = choice.choiceDispatchKeyEv.expr.toBriefXML()
+ val before = "'{xs:string("
+ val after = ")}'"
+ val relativePath = if (expr.startsWith(before) && expr.endsWith(after))
+ expr.substring(before.length, expr.length - after.length) else expr
+ val normalizedURI = new URI(absoluteSlashPath + "/" + relativePath).normalize
+ normalizedURI.getPath.substring(1)
+ case _ => ""
+ }
+ // Strip namespace prefixes since C code uses only local names (for now...)
+ val localDispatchSlashPath = dispatchSlashPath.replaceAll("/[^:]+:", "/")
+ val res = localDispatchSlashPath.replace('/', '.')
+ res
+ }
Review comment:
As this runtime2 matures, I wonder if eventually we would want to add code generate support to the DPath. This way you can take advantage all of the DPath expression capabilities like type checking/conversion. But then rather than creating our runtime1 objects, you can generate code. This likely requires a decent amount of refactoring of the DPath, but probably somethign worth keeping in the back of our minds. As is, this feels pretty fragile, and I'm not even sure it will error if a dispatch expression doesn't match the supported format.
##########
File path: daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala
##########
@@ -162,10 +322,31 @@ class CodeGeneratorState {
def addComplexTypeStatements(child: ElementBase): Unit = {
val C = localName(child)
val e = child.name
+ val hasChoice = structs.top.initChoiceStatements.nonEmpty
Review comment:
If I'm reading this right, it seems this only supports direct dispatch choices that are the model group of a complex type, e.g.:
```xml
<xs:element name="foo">
<xs:complexType>
<xs:choice dfdl:choiceDispatchKey="...">
...
</xs:choice>
</xs:complexType>
</xs:element>
```
So sequences with "anonymous" choices are not suppported? I guess supporting those adds more complications since you currently use ``_choice`` to figure out which union member is used, and anonmous choices means you could have multiple unions and _choice's, which someone need a distint name?
Does this distint name issue also arise in sequence with multiple elements with the same name? For example:
```xml
<xs:element name="foo">
<xs:complexType>
<xs:sequence>
<xs:element name="a" ... />
<xs:element name="b" ... />
<xs:element name="a" ... />
</xs:choice>
</xs:complexType>
</xs:element>
```
Would this currently result in to struct members with the name "a" and cause an error?
##########
File path: daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/ingress_xdcc_bw.tdml
##########
@@ -0,0 +1,196 @@
+<?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-runtime2"
+ defaultImplementations="daffodil daffodil-runtime2"
+ defaultRoundTrip="none"
+ description="TDML tests for ingress_xdcc_bw"
+ 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="ingress_xdcc_bw parse test 111"
+ model="ingress_xdcc_bw.dfdl.xsd"
+ name="ingress_xdcc_bw_parse_111"
+ root="GapsPDU">
+ <tdml:document>
+ <tdml:documentPart type="file">ingress_xdcc_bw_parse_111.dat</tdml:documentPart>
+ </tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ingress_xdcc_bw_unparse_111.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:unparserTestCase
+ description="ingress_xdcc_bw unparse test 111"
+ model="ingress_xdcc_bw.dfdl.xsd"
+ name="ingress_xdcc_bw_unparse_111"
+ root="GapsPDU">
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ingress_xdcc_bw_unparse_111.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>
+ <tdml:documentPart type="file">ingress_xdcc_bw_parse_111.dat</tdml:documentPart>
+ </tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:parserTestCase
+ description="ingress_xdcc_bw parse test 112"
+ model="ingress_xdcc_bw.dfdl.xsd"
+ name="ingress_xdcc_bw_parse_112"
+ root="GapsPDU">
+ <tdml:document>
+ <tdml:documentPart type="file">ingress_xdcc_bw_parse_112.dat</tdml:documentPart>
+ </tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ingress_xdcc_bw_unparse_112.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:unparserTestCase
+ description="ingress_xdcc_bw unparse test 112"
+ model="ingress_xdcc_bw.dfdl.xsd"
+ name="ingress_xdcc_bw_unparse_112"
+ root="GapsPDU">
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ingress_xdcc_bw_unparse_112.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>
+ <tdml:documentPart type="file">ingress_xdcc_bw_parse_112.dat</tdml:documentPart>
+ </tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:parserTestCase
+ description="ingress_xdcc_bw parse test 113"
+ model="ingress_xdcc_bw.dfdl.xsd"
+ name="ingress_xdcc_bw_parse_113"
+ root="GapsPDU">
+ <tdml:document>
+ <tdml:documentPart type="file">ingress_xdcc_bw_parse_113.dat</tdml:documentPart>
+ </tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ingress_xdcc_bw_unparse_113.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:unparserTestCase
+ description="ingress_xdcc_bw unparse test 113"
+ model="ingress_xdcc_bw.dfdl.xsd"
+ name="ingress_xdcc_bw_unparse_113"
+ root="GapsPDU">
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ingress_xdcc_bw_unparse_113.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>
+ <tdml:documentPart type="file">ingress_xdcc_bw_parse_113.dat</tdml:documentPart>
+ </tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:parserTestCase
+ description="ingress_xdcc_bw parse test 114"
+ model="ingress_xdcc_bw.dfdl.xsd"
+ name="ingress_xdcc_bw_parse_114"
+ root="GapsPDU">
+ <tdml:document>
+ <tdml:documentPart type="file">ingress_xdcc_bw_parse_114.dat</tdml:documentPart>
+ </tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ingress_xdcc_bw_unparse_114.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:unparserTestCase
+ description="ingress_xdcc_bw unparse test 114"
+ model="ingress_xdcc_bw.dfdl.xsd"
+ name="ingress_xdcc_bw_unparse_114"
+ root="GapsPDU">
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ingress_xdcc_bw_unparse_114.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>
+ <tdml:documentPart type="file">ingress_xdcc_bw_parse_114.dat</tdml:documentPart>
+ </tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:parserTestCase
+ description="ingress_xdcc_bw parse test 115"
+ model="ingress_xdcc_bw.dfdl.xsd"
+ name="ingress_xdcc_bw_parse_115"
+ root="GapsPDU">
+ <tdml:document>
+ <tdml:documentPart type="file">ingress_xdcc_bw_parse_115.dat</tdml:documentPart>
+ </tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ingress_xdcc_bw_unparse_115.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:unparserTestCase
+ description="ingress_xdcc_bw unparse test 115"
+ model="ingress_xdcc_bw.dfdl.xsd"
+ name="ingress_xdcc_bw_unparse_115"
+ root="GapsPDU">
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ingress_xdcc_bw_unparse_115.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>
+ <tdml:documentPart type="file">ingress_xdcc_bw_parse_115.dat</tdml:documentPart>
+ </tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:parserTestCase
+ description="ingress_xdcc_bw parse test 116"
+ model="ingress_xdcc_bw.dfdl.xsd"
+ name="ingress_xdcc_bw_parse_116"
+ root="GapsPDU">
+ <tdml:document>
+ <tdml:documentPart type="file">ingress_xdcc_bw_parse_116.dat</tdml:documentPart>
+ </tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ingress_xdcc_bw_unparse_116.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:unparserTestCase
+ description="ingress_xdcc_bw unparse test 116"
+ model="ingress_xdcc_bw.dfdl.xsd"
+ name="ingress_xdcc_bw_unparse_116"
+ root="GapsPDU">
+ <tdml:infoset>
+ <tdml:dfdlInfoset type="file">ingress_xdcc_bw_unparse_116.xml</tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>
+ <tdml:documentPart type="file">ingress_xdcc_bw_parse_116.dat</tdml:documentPart>
+ </tdml:document>
+ </tdml:unparserTestCase>
+
+</tdml:testSuite>
Review comment:
Does it make sense to put all these tests in daffodil-tests, or somewhere not runtime2 specific? It makes sense that they are in here because they're made to ensure that runtime2 works, but these schemas aren't runtime2 specific. They should run just fine in DFDL runtime, or maybe even the IBM DFDL runtime.
I would think that maybe runtime2 should only contain things like unit tests that are truely specific to runtime2. For example, tests that generate code and then examing the generated code to make sure the right thing is generated. Not sure we need that kind of test, but that's the kind of tests that should be in runtime2 rather than daffodil-test.
----------------------------------------------------------------
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