mbeckerle commented on code in PR #901:
URL: https://github.com/apache/daffodil/pull/901#discussion_r1073783098
##########
daffodil-core/src/main/scala/org/apache/daffodil/grammar/AlignedMixin.scala:
##########
@@ -138,7 +138,7 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
// The separator is text in some encoding, might not be the same as this
term's encoding, and
// the alignment will be left where that text leaves it.
//
- private lazy val priorAlignmentApprox: AlignmentMultipleOf =
LV('priorAlignmentApprox) {
+ lazy val priorAlignmentApprox: AlignmentMultipleOf =
LV('priorAlignmentApprox) {
Review Comment:
I don't think you want to expose these. I think you want to define a simpler
concept.
I think you just want to call isKnownToBeAligned (already public). If false,
you must generate a call to an operation that advances to achieve the required
alignment (which is always 4 as I understand it.)
##########
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala:
##########
@@ -404,76 +542,117 @@ class CodeGeneratorState(private val root: ElementBase) {
finalStructs += struct
}
- def addSimpleTypeStatements(initERDStatement: String, initSelfStatement:
String, parseStatement: String, unparseStatement: String): Unit = {
- if (initERDStatement.nonEmpty) structs.top.initERDStatements +=
initERDStatement
- if (initSelfStatement.nonEmpty) structs.top.initSelfStatements +=
initSelfStatement
+ // Add C code to initialize, parse, and unparse a primitive value element
+ def addSimpleTypeStatements(initERDStatement: String, parseStatement:
String, unparseStatement: String): Unit = {
+ if (initERDStatement.nonEmpty) {
+ if (hasChoice)
+ structs.top.initChoiceStatements += initERDStatement
+ else
+ structs.top.initERDStatements += initERDStatement
+ }
if (parseStatement.nonEmpty) structs.top.parserStatements += parseStatement
if (unparseStatement.nonEmpty) structs.top.unparserStatements +=
unparseStatement
}
- def addComplexTypeStatements(child: ElementBase): Unit = {
+ // Generate C code to initialize, parse, and unparse a child element
+ def addPerChildStatements(child: ElementBase): Unit = {
val C = cStructName(child)
- val e = child.name
- val hasChoice = structs.top.initChoiceStatements.nonEmpty
- val arraySize = if (child.occursCountKind == OccursCountKind.Fixed)
child.maxOccurs else 0
+ val e = child.namedQName.local
+ // Generate a choice statement case if the child is in a choice element
+ val hasChoice = structs.top.initChoiceStatements.nonEmpty
if (hasChoice) {
+ val position = child.position
structs.top.initChoiceStatements ++=
ChoiceBranchKeyCooker.convertConstant(
child.choiceBranchKey, child, forUnparse = false).map { key => s"
case $key:"}
-
- val offset = child.position - 1
- val initChoiceStatement = s" instance->_choice = $offset;"
- val parseStatement = s" case $offset:"
- val unparseStatement = s" case $offset:"
-
- structs.top.initChoiceStatements += initChoiceStatement
- structs.top.parserStatements += parseStatement
- structs.top.unparserStatements += unparseStatement
+ structs.top.initChoiceStatements += s" instance->_choice =
$position;"
+ structs.top.parserStatements += s" case $position:"
+ structs.top.unparserStatements += s" case $position:"
}
+ val indent1 = if (hasChoice) " " else ""
- def addStatements(deref: String): Unit = {
- val initChoiceStatement = s" ${C}_initERD(&instance->$e$deref);"
- val initERDStatement = s" ${C}_initERD(&instance->$e$deref);"
- val initSelfStatement = s" ${C}_initSelf(&instance->$e$deref);"
- val moreIndent = if (hasChoice) " " else ""
- val parseStatement =
- s"""$moreIndent ${C}_parseSelf(&instance->$e$deref, pstate);
- |$moreIndent if (pstate->error) return;""".stripMargin
- val unparseStatement =
- s"""$moreIndent ${C}_unparseSelf(&instance->$e$deref, ustate);
- |$moreIndent if (ustate->error) return;""".stripMargin
+ // Add an array if the child occurs multiple times
+ val hasArray = arrayMaxOccurs(child) > 0
+ val deref = if (hasArray) "[i]" else ""
+ val indent2 = if (hasArray) " " else ""
+ if (hasArray) pushArray(child)
+ // Generate statements to initialize, parse, and unparse the child
+ Runtime2CodeGenerator.generateCode(child.enclosedElement, this)
+ if (!child.isSimpleType) {
if (hasChoice)
- structs.top.initChoiceStatements += initChoiceStatement
+ structs.top.initChoiceStatements += s"$indent2
${C}_initERD(&instance->$e$deref, (InfosetBase *)instance);"
else
- structs.top.initERDStatements += initERDStatement
- structs.top.initSelfStatements += initSelfStatement
- structs.top.parserStatements += parseStatement
- structs.top.unparserStatements += unparseStatement
+ structs.top.initERDStatements += s"$indent2
${C}_initERD(&instance->$e$deref, (InfosetBase *)instance);"
+ structs.top.parserStatements +=
+ s"""$indent1$indent2 ${C}_parseSelf(&instance->$e$deref, pstate);
+ |$indent1$indent2 if (pstate->error) return;""".stripMargin
+ structs.top.unparserStatements +=
+ s"""$indent1$indent2 ${C}_unparseSelf(&instance->$e$deref, ustate);
+ |$indent1$indent2 if (ustate->error) return;""".stripMargin
}
- if (arraySize > 0)
- for (i <- 0 until arraySize)
- addStatements(s"[$i]")
- else
- addStatements("")
- if (hasChoice) {
- val initChoiceStatement = s" break;"
- val parseStatement = s" break;"
- val unparseStatement = s" break;"
+ // Generate statements to align the next element
+ val mustAlign = child.endingAlignmentApprox.nBits >
child.priorAlignmentApprox.nBits
Review Comment:
I'd like to suggest that Daffodil's schema compiler should provide an
available option which is to require all alignment to be static except for
alignment after something that is truly variable length. Runtime 2 would then
always require this to be enabled. But I think this would be popular for any
schema author.
For example:
this would not be allowed:
```
<sequence dfdl:alignment="4">
<element dfdl:alignment="4" .fixed length element of length 4.../>
<element dfdl:alignment="8" .fixed length element of length 8 ../>
</sequence>
```
because the 2nd element may or may not require 4 bytes of alignment fill
depending on the actual alignment of the starting sequence. This is a complete
pain in the neck because you can't even tell the length of this structure
without knowing where it starts despite the fact that it is all fixed length
stuff. (This is called the "interior aligment problem" lots of things that
appear fixed length become variable length.)
I think we should have an option where we just say no to this. We have a
property dfdlx:alignmentKind. Current values are 'manual' (no alignment regions
ever inserted) or 'automatic' (everything including interior alignment
allowed).
I'm suggesting we add another enum value for this of 'onlyForVariableLength'
which says alignment must be achieved manually by the schema author (using
things like leading/trailing skip properties) except after variable-length
elements.
Daffodil's core isKnownToBeAligned is false for the 2nd element in the
example above even if it is true for the surrounding sequence.
So when dfdl:alignmentKind is 'onlyForVariableLength' we would make it an
SDE if any element is not knownToBeAligned unless it is preceded by something
variable length. The SDE could require the author to increase the alignment of
the surrounding sequence to 8, and then they could fix an SDE on the 2nd
element by adding a leading skip (or trailing to the prior).
##########
daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/variablelen.dfdl.xsd:
##########
@@ -0,0 +1,121 @@
+<?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.
+-->
+
+<xs:schema
+ elementFormDefault="qualified"
+ xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/";
+ xmlns:xs="http://www.w3.org/2001/XMLSchema";>
+
+ <!-- Binary data format bindings -->
+
+ <xs:include
schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <xs:annotation>
+ <xs:appinfo source="http://www.ogf.org/dfdl/";>
+ <dfdl:format
+ ref="GeneralFormat"
+ binaryBooleanFalseRep="0"
+ binaryBooleanTrueRep="1"
+ fillByte="%NUL;"
+ prefixIncludesPrefixLength="no"
+ representation="binary"/>
+ </xs:appinfo>
+ </xs:annotation>
+
+ <!-- Types for testing -->
+
+ <!-- Unsuitable since variable length array must allow 0 to 16 numbers -->
Review Comment:
What do you mean by unsuitable? For what is it unsuitable?
--
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]
