stevedlawrence commented on code in PR #1176:
URL: https://github.com/apache/daffodil/pull/1176#discussion_r1517689566
##########
daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1/FixedLengthLayer.scala:
##########
@@ -42,33 +40,35 @@ import org.apache.commons.io.IOUtils
* This length is enforced on both parsing and unparsing the layer.
* There are no output/result DFDL variables from this layer.
*/
-final class FixedLengthLayer(fixedLength: JLong)
+final class FixedLengthLayer
extends Layer("fixedLength", "urn:org.apache.daffodil.layers.fixedLength") {
- Assert.invariant(fixedLength > 0)
+ private var fixedLength: Long = 0
- /** Required for SPI class loading */
- def this() = this(1)
-
- private def maxFixedLength = Short.MaxValue
-
- override def wrapLayerInput(jis: InputStream, lr: LayerRuntime): InputStream
= {
+ def setLayerVariableParameters(fixedLength: Long): Unit = {
+ this.fixedLength = fixedLength
+ }
+ private def check(lr: LayerRuntime): Unit = {
+ if (fixedLength < 1)
Review Comment:
Question above about the possibiity for setLayerVariableParameters to
perform validation checks would mean this check function can go away, and wrap
functions can always assume valid state.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/ChecksumLayerBase.scala:
##########
@@ -38,11 +38,35 @@ import org.apache.commons.io.IOUtils
abstract class ChecksumLayerBase(
localName: String,
namespace: String,
- val length: Int,
) extends Layer(localName, namespace) {
private var checksum: Int = -1
+ private var length: Int = -1
+ private var isInitialized: Boolean = false
+
+ final def getLength: Int = length
+ final protected def setLength(len: Int): Unit = {
+ this.length = len
+ }
+
+ /**
+ * Initializes the layer.
+ *
+ * This method is called to ensure that `setLength` has been called before
the streams are created.
+ * It can be overridden in subclasses, but those must run the
`super.init(lr)`
+ *
+ * @param lr The LayerRuntime instance.
+ */
+ protected def init(lr: LayerRuntime): Unit = {
Review Comment:
If setVariable can check errors I think init also goes away?
Then setLength would create failures if length is <= 0 however setVariable
would?
I guess we still need a check to make sure implementations actually call
setLength, so the wrap functions do need some kind of initialization check.
Maybe you could change the var to a JInt that is initialized to null, and the
wrap functions could do something simple like `Assert.invariant(length !=
null)`. Just trying to think of ways to makes things as simple as possible.
Also, it feels like if a user doesn't cal setLength, that wants to be an
error more aggressive than a processing error. That's a bug in the layer and
needs to be fixed.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/api/Layer.java:
##########
@@ -63,11 +63,11 @@
* <p/>
* A layer that wants to return a value after the layer algorithm completes
defines a special recognizable
* getter method. The name of the getter is formed from prefixing the DFDL
variable name with the string
- * 'getDFDLResultVariable_'. The return type of the getter must match the type
of the variable.
+ * 'getLayerVariableResult_'. The return type of the getter must match the
type of the variable.
* <p/>
* For example, a result value getter for a DFDL variable named 'checksum' of
type xs:unsignedShort would be:
* <pre>
- * Int getDFDLResultVariable_checksum() {
+ * Int getLayerVariableResult_checksum() {
Review Comment:
Do these set/getVariable functions need o be public, or can reflection still
find them if they are package private?
Also, it might be worth using `int` instead of `Int` since this is javadoc.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/LayerFactory.scala:
##########
@@ -65,73 +72,87 @@ object LayerFactory {
// find another constructor and check that there is an argument
// corresponding to each of the layer variables.
val c = protoLayer.getClass
- val allConstructors = c.getConstructors.toSeq
-
- val constructor: Constructor[_] =
- if (allConstructors.length == 1) {
- // There is only the default constructor.
- // That's ok if there are no variables for the layer, which we check
later.
- allConstructors.head
- } else if (allConstructors.length == 2) {
- allConstructors.filter(_.getParameterCount > 0).head
- } else {
- def tooManyConstructorsMsg: String = {
- s"""Layer class $c has multiple non-default constructors. It
should have a default (no args)
- | constructor and a single additional constructor with
arguments for
- | each of the layer's parameter variables.""".stripMargin
- }
-
- lrd.context.SDE(tooManyConstructorsMsg)
- }
+ val constructor = c.getConstructors.find { c => c.getParameterCount == 0
}.getOrElse {
+ Assert.invariantFailed("SPI-loaded class without default constructor?
Not possible.")
+ }
+ val allMethods = c.getMethods
+ val optParamSetter = allMethods.find { _.getName == varParamSetter }
+ val allVarResultGetters =
+ ListSet(allMethods.filter { m =>
+ val nom = m.getName
+ nom.startsWith(varResultPrefix)
+ }.toSeq: _*)
- if (lrd.vmap.isEmpty && allConstructors.length == 1) {
+ if (lrd.vmap.isEmpty && optParamSetter.isEmpty &&
allVarResultGetters.isEmpty) {
// there are no vars, we're done
- new LayerVarsRuntime(constructor, Nil, Nil)
+ new LayerVarsRuntime(constructor, None, Nil, Nil)
} else {
- // there is a constructor with args that are supposed to correspond to
bound vars
- val params = constructor.getParameters.toSeq
- val nParams = params.length
- val nVars = lrd.vmap.size
+ val allLayerVRDs = ListSet(lrd.vmap.toSeq.map(_._2): _*)
+ // there is either a params setter, a result getter, or both.
+ val paramVRDs: Seq[VariableRuntimeData] =
+ optParamSetter.toSeq.flatMap { paramSetter =>
+ // there is a param setter with args that are supposed to
correspond to bound vars
+ val params = paramSetter.getParameters.toSeq
- val paramTypes = constructor.getParameterTypes.toSeq
- val paramVRDs = params.map { p =>
- lrd.vmap.getOrElse(
- p.getName,
- lrd.context.SDE(
- s"No layer DFDL variable named '$p.getName' was found in
namespace ${lrd.namespace}.",
- ),
- )
- }.toSeq
+ val paramTypes = paramSetter.getParameterTypes.toSeq
+ val pVRDs: Seq[VariableRuntimeData] = params.map { p =>
+ lrd.vmap.getOrElse(
+ p.getName,
+ lrd.context.SDE(
+ s"No layer DFDL variable named '$p.getName' was found in
namespace ${lrd.namespace}.",
+ ),
+ )
+ }.toSeq
- // Now we deal with the result getters and the corresponding vars
- //
- val allLayerVRDs = ListSet(lrd.vmap.toSeq.map(_._2): _*)
- //
- // verify only Int and String are allowed simple types for now.
- // The rest of the code beyond here doesn't care what the type is, but
we don't want to
- // create a bunch of test code for layer things that aren't needed.
- //
- allLayerVRDs.foreach { vrd =>
- lrd.context.schemaDefinitionUnless(
- vrd.primType == PrimType.String ||
- vrd.primType == PrimType.Int ||
- vrd.primType == PrimType.UnsignedShort,
- s"Layer variable ${vrd.globalQName.toQNameString} of type
${vrd.primType.dfdlType} must be of type xs:int, xs:unsignedShort, or
xs:string.",
- )
- }
+ // Now we deal with the result getters and the corresponding vars
+ //
+
+ //
+ // verify only Int and String are allowed simple types for now.
+ // The rest of the code beyond here doesn't care what the type is,
but we don't want to
+ // create a bunch of test code for layer things that aren't needed.
Review Comment:
As far as test code, we could have a single layer that has one variable for
each type we want to support. Then testing each type just requires adding a new
dfdl:defineVariable and a new parameter to setVariable. Doing that now seems
worth it to avoid someone coming a long and saying "I really need unsignedLong"
or something, especially considering the rest of the code doesn't care. Or at
least if we add that test now, we have a place to test to extend when we do
want to add support for new types.
##########
daffodil-test/src/test/scala/org/apache/daffodil/runtime1/layers/IPv4ChecksumLayer.scala:
##########
@@ -36,38 +36,42 @@ final class IPv4ChecksumLayer()
extends ChecksumLayer(
"IPv4Checksum",
"urn:org.apache.daffodil.layers.IPv4Checksum",
- 20,
) {
- /**
- * Result DFDL variable value getter. This getter is called to obtain the
value for,
- * and populate the DFDL variable named checkDigit, in the layer's target
namespace.
- * @return the check digit value
- */
- def getDFDLResultVariable_IPv4Checksum: JInt =
- getChecksum
-
/**
* This layer is always exactly 20 bytes long.
*/
private def lenInBytes = 20
private def chksumShortIndex = 5
+ override protected def init(lr: LayerRuntime): Unit = {
+ setLength(lenInBytes)
+ super.init(lr)
+ }
+
+ /**
+ * Result DFDL variable value getter. This getter is called to obtain the
value for,
+ * and populate the DFDL variable named checkDigit, in the layer's target
namespace.
+ * @return the check digit value
+ */
+ def getLayerVariableResult_IPv4Checksum: JInt = getChecksum
+
/**
* Computes the checksum value.
* When unparsing this also modifies the output bytes to have the checksum
at the
* middle location as per the IPv4 spec.
* The LayerChecksumMixin assigns the computed checksum value to the first
variable.
- * @param layerCompileInfo structure providing access to state such as
variables, when needed.
+ * @param layerRuntime structure providing access to state such as
variables, when needed.
* @param isUnparse true if the checksum is for unparsing.
* @param byteBuffer contains the 20 bytes to be used to compute this
checksum.
* @return the computed checksum value as a 32-bit signed Int.
*/
override def compute(
- layerCompileInfo: LayerRuntime,
+ layerRuntime: LayerRuntime,
isUnparse: Boolean,
byteBuffer: ByteBuffer,
): Int = {
+ init(layerRuntime)
Review Comment:
I'm not a huge fan of this init, it feels an awful lot like the check()
function that we tried to remove. Maybe it's required in this case? I did
mention a possible alternative in another comment where the set variable
function calls setLength, but I guess the issue here is this layer don't have
the a set variable function to call setLength since this layer has no variables?
A thought, does it work to have an empty set variable functions? For
example, maybe this checksum layer implementation could do:
```scala
def setLayerVariableParameters(): Unit = {
setLength(20)
}
```
So the documentation for ChecksumLayer api says all implementations must
implement a setLayerVariableParameters function and must call setLength(). If
the layer has no variables, the function can have an empty parameter list.
So setLayerVariablParameters kindof becomes a layers `init()` function, and
it can be parameterized by variables or not?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/LayerFactory.scala:
##########
@@ -65,73 +72,87 @@ object LayerFactory {
// find another constructor and check that there is an argument
// corresponding to each of the layer variables.
val c = protoLayer.getClass
- val allConstructors = c.getConstructors.toSeq
-
- val constructor: Constructor[_] =
- if (allConstructors.length == 1) {
- // There is only the default constructor.
- // That's ok if there are no variables for the layer, which we check
later.
- allConstructors.head
- } else if (allConstructors.length == 2) {
- allConstructors.filter(_.getParameterCount > 0).head
- } else {
- def tooManyConstructorsMsg: String = {
- s"""Layer class $c has multiple non-default constructors. It
should have a default (no args)
- | constructor and a single additional constructor with
arguments for
- | each of the layer's parameter variables.""".stripMargin
- }
-
- lrd.context.SDE(tooManyConstructorsMsg)
- }
+ val constructor = c.getConstructors.find { c => c.getParameterCount == 0
}.getOrElse {
+ Assert.invariantFailed("SPI-loaded class without default constructor?
Not possible.")
+ }
+ val allMethods = c.getMethods
+ val optParamSetter = allMethods.find { _.getName == varParamSetter }
Review Comment:
Should we check if there there are multiple overloaded varParamSetter an
error?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/api/ChecksumLayer.java:
##########
@@ -23,29 +23,29 @@
/**
* A checksum layer computes some sort of integer value from a region of the
data stream. The term checksum is
* used generically here to subsume all sorts of CRCs, check digits, data
hash, and digest calculations.
- *
+ * <p/>
* This base is suitable only for checksums computed over small sections of
data, not large data streams or whole files.
- *
+ * <p/>
* The entire region of data the checksum is being computed over, will be
pulled into a byte buffer in memory.
- *
+ * <p/>
* The resulting checksum is the return value of the compute method.
- *
+ * <p/>
* This is delivered into a DFDL variable for use by the DFDL schema. This
variable can have any name
* such as 'crc', 'digest', or 'dataHash'.
- *
+ * <p/>
* The derived implementation class must also define a getter method based on
the name of the DFDL variable which
* will be assigned with the checksum value. For example if the checksum is
actually a specific digest/hash calculation
* and the DFDL variable is named 'digest', then this getter must be defined:
- *
- * Int getDFDLResultVariable_digest() { return getChecksum() }
- *
+ * <p/>
+ * Int getLayerVariableResult_digest() { return getChecksum() }
+ * <p/>
* This will be called automatically to retrieve the integer value that was
returned from the `compute` method, and
* the DFDL variable 'digest' will be assigned that value.
*/
public abstract class ChecksumLayer extends ChecksumLayerBase {
Review Comment:
Do we need a comment that implementations must call `setLength` prior to
wrap being called?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/LayerFactory.scala:
##########
@@ -65,73 +72,87 @@ object LayerFactory {
// find another constructor and check that there is an argument
// corresponding to each of the layer variables.
val c = protoLayer.getClass
- val allConstructors = c.getConstructors.toSeq
-
- val constructor: Constructor[_] =
- if (allConstructors.length == 1) {
- // There is only the default constructor.
- // That's ok if there are no variables for the layer, which we check
later.
- allConstructors.head
- } else if (allConstructors.length == 2) {
- allConstructors.filter(_.getParameterCount > 0).head
- } else {
- def tooManyConstructorsMsg: String = {
- s"""Layer class $c has multiple non-default constructors. It
should have a default (no args)
- | constructor and a single additional constructor with
arguments for
- | each of the layer's parameter variables.""".stripMargin
- }
-
- lrd.context.SDE(tooManyConstructorsMsg)
- }
+ val constructor = c.getConstructors.find { c => c.getParameterCount == 0
}.getOrElse {
Review Comment:
Can this be simplified to this to get the zero arg constructor:
```scala
val constructor = c.getConstructor()
```
In order for SPI to have found the layer and gotten this far, and created an
instance this must return something, right?
##########
daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1/BoundaryMarkLayer.scala:
##########
@@ -45,21 +45,29 @@ import org.apache.daffodil.runtime1.layers.api.LayerRuntime
* disrupt the search for the boundary mark string.
*
* This layer does not populate any DFDL variables with results.
+ *
* This layer defines two DFDL variables which provide required parameters.
- * @param boundaryMark a string which is the boundary marker. Searching for
this string is
- * done without any notion of escaping. When parsing the
data in the
- * layer is up to but not including that of the boundary
mark string,
- * which is removed from the data-stream on parsing, and
inserted into
- * the data stream on unparsing.
- * @param layerEncoding a string which is the name of the character set
encoding used to
- * decode characters during the search for the boundary
mark, and used
- * to encode characters when inserting the boundary mark
when unparsing.
*/
-final class BoundaryMarkLayer(boundaryMark: String, layerEncoding: String)
+final class BoundaryMarkLayer
extends Layer("boundaryMark", "urn:org.apache.daffodil.layers.boundaryMark")
{
- /** no arg contructor required for SPI class loading */
- def this() = this("dummy", "ascii")
+ private var boundaryMark: String = _
+ private var layerEncoding: String = _
+
+ /**
+ * @param boundaryMark a string which is the boundary marker. Searching for
this string is
+ * done without any notion of escaping. When parsing
the data in the
+ * layer is up to but not including that of the
boundary mark string,
+ * which is removed from the data-stream on parsing,
and inserted into
+ * the data stream on unparsing.
+ * @param layerEncoding a string which is the name of the character set
encoding used to
+ * decode characters during the search for the boundary
mark, and used
+ * to encode characters when inserting the boundary
mark when unparsing.
+ */
+ def setLayerVariableParameters(boundaryMark: String, layerEncoding: String):
Unit = {
+ this.boundaryMark = boundaryMark
+ this.layerEncoding = layerEncoding
Review Comment:
Should we pass in LayerRuntime into this function for so users can throw
errors here? For example, this might want to be something like:
```scala
private var boundaryMark: String = _
private var charset: Charset = _
def setLayerVariableParameters(lr: LayerRuntime, boundryMark: String,
layerEncoding: String): Unit = {
this.boundaryMark = boundaryMake
this.charset = try {
Charset.forName(layerEncoding)
} catch {
case e: Exception => lr.runtimeSchemaDefinitionError(...)
}
}
```
Or alternatively, maybe `setLayerVariableParameters` can throw an exception
(maybe IllegalArgumentException or a custom LayerBadVariableException?) if any
variables values don't make sense for a layer? And the layer backend swould
catch that and convert it to an SDE?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/api/Layer.java:
##########
@@ -63,11 +63,11 @@
* <p/>
Review Comment:
THis doc needs updating to use the new setVariable method.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/LayerFactory.scala:
##########
@@ -0,0 +1,326 @@
+/*
+ * 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.runtime1.layers
+
+import java.lang.reflect.Constructor
+import java.lang.reflect.Method
+import scala.collection.immutable.ListSet
+import scala.collection.mutable
+
+import org.apache.daffodil.lib.exceptions.Assert
+import org.apache.daffodil.runtime1.dpath.NodeInfo.PrimType
+import org.apache.daffodil.runtime1.infoset.DataValue
+import org.apache.daffodil.runtime1.layers.api.Layer
+import org.apache.daffodil.runtime1.layers.api.LayerException
+import org.apache.daffodil.runtime1.processors.VariableRuntimeData
+
+object LayerFactory {
+
+ /** cache that maps spiName of layer to the LayerVarsRuntime */
+ private lazy val alreadyCheckedLayers =
+ new mutable.LinkedHashMap[String, LayerVarsRuntime]()
+
+ /**
+ * Computes the things that can be computed once-only to make calling the
constructor
+ * and passing the parameter variables faster at runtime. This also does all
the
+ * checking that the constructor has an argument for each variable of
properly matching type, and
+ * has a getter for each result variable, again returning the proper type.
+ *
+ * This is called at schema compile time to ensure the layer code is
properly defined and matches
+ * the layer variable definitions.
+ *
+ * It is called again at runtime after the Layer class is loaded by the SPI
+ * to ensure that the loaded layer class constructor signature at least
matches the layer
+ * variables defined in the schema.
+ * @param lrd
+ * @param protoLayer the layer instance allocated by the SPI loader
(zero-arg constructed)
+ * @return
+ */
+ def computeLayerVarsRuntime(lrd: LayerRuntimeData, protoLayer: Layer):
LayerVarsRuntime = {
+ val optLayerVarsRuntime = alreadyCheckedLayers.get(protoLayer.name())
+ optLayerVarsRuntime.getOrElse {
+ // we know there is a default zero arg constructor
+ // find another constructor and check that there is an argument
+ // corresponding to each of the layer variables.
+ val c = protoLayer.getClass
+ val allConstructors = c.getConstructors.toSeq
+
+ val constructor: Constructor[_] =
+ if (allConstructors.length == 1) {
+ // There is only the default constructor.
+ // That's ok if there are no variables for the layer, which we check
later.
+ allConstructors.head
+ } else if (allConstructors.length == 2) {
+ allConstructors.filter(_.getParameterCount > 0).head
+ } else {
+ def tooManyConstructorsMsg: String = {
+ s"""Layer class $c has multiple non-default constructors. It
should have a default (no args)
+ | constructor and a single additional constructor with
arguments for
+ | each of the layer's parameter variables.""".stripMargin
+ }
+ lrd.context.SDE(tooManyConstructorsMsg)
+ }
+
+ if (lrd.vmap.isEmpty && allConstructors.length == 1) {
+ // there are no vars, we're done
+ new LayerVarsRuntime(constructor, Nil, Nil)
+ } else {
+ // there is a constructor with args that are supposed to correspond to
bound vars
+ val params = constructor.getParameters.toSeq
+ val nParams = params.length
+ val nVars = lrd.vmap.size
+
+ val paramTypes = constructor.getParameterTypes.toSeq
+ val paramVRDs = params.map { p =>
+ lrd.vmap.getOrElse(
+ p.getName,
+ lrd.context.SDE(s"No layer DFDL variable named '$p.getName' was
found."),
+ )
+ }.toSeq
+
+ // Now we deal with the result getters and the corresponding vars
+ //
+ val allLayerVRDs = ListSet(lrd.vmap.toSeq.map(_._2): _*)
+ val returnVRDs = allLayerVRDs -- paramVRDs // set subtraction
Review Comment:
This was resolved without comment, just wanted to make sure you saw it.
I don't feel strongly about it, but it might simplify the logic if we didn't
require that all variables had to be either be parameters or results. For
example, getting the getting paris could be something like
```scala
val resultVarParis = allVarResultGetters.map { method =>
val varName = method.getName.drop(varResultPrefix.length)
val vrd = vmap.find { QName(layer.getNamespace, varName)
}.getOrElse(SDE(...))
val vrdClass = PrimType.toJavaType(vrd.primType.dfdlType)
schemaDefinitionUnless(compatibleTypes(vrdClass, method.getReturnType),
...)
(valVRD, method)
}
```
I'm sure that's a bit of an over simplification, but it it feels like it's
less complex? Note that LayerRuntimeData doesn't even need the vmap anymore, we
can just directly look up variables in the normal vmap, so that goes away too.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/LayerFactory.scala:
##########
@@ -65,73 +72,87 @@ object LayerFactory {
// find another constructor and check that there is an argument
// corresponding to each of the layer variables.
val c = protoLayer.getClass
- val allConstructors = c.getConstructors.toSeq
-
- val constructor: Constructor[_] =
- if (allConstructors.length == 1) {
- // There is only the default constructor.
- // That's ok if there are no variables for the layer, which we check
later.
- allConstructors.head
- } else if (allConstructors.length == 2) {
- allConstructors.filter(_.getParameterCount > 0).head
- } else {
- def tooManyConstructorsMsg: String = {
- s"""Layer class $c has multiple non-default constructors. It
should have a default (no args)
- | constructor and a single additional constructor with
arguments for
- | each of the layer's parameter variables.""".stripMargin
- }
-
- lrd.context.SDE(tooManyConstructorsMsg)
- }
+ val constructor = c.getConstructors.find { c => c.getParameterCount == 0
}.getOrElse {
+ Assert.invariantFailed("SPI-loaded class without default constructor?
Not possible.")
+ }
+ val allMethods = c.getMethods
+ val optParamSetter = allMethods.find { _.getName == varParamSetter }
+ val allVarResultGetters =
+ ListSet(allMethods.filter { m =>
+ val nom = m.getName
+ nom.startsWith(varResultPrefix)
+ }.toSeq: _*)
- if (lrd.vmap.isEmpty && allConstructors.length == 1) {
+ if (lrd.vmap.isEmpty && optParamSetter.isEmpty &&
allVarResultGetters.isEmpty) {
// there are no vars, we're done
- new LayerVarsRuntime(constructor, Nil, Nil)
+ new LayerVarsRuntime(constructor, None, Nil, Nil)
} else {
- // there is a constructor with args that are supposed to correspond to
bound vars
- val params = constructor.getParameters.toSeq
- val nParams = params.length
- val nVars = lrd.vmap.size
+ val allLayerVRDs = ListSet(lrd.vmap.toSeq.map(_._2): _*)
+ // there is either a params setter, a result getter, or both.
+ val paramVRDs: Seq[VariableRuntimeData] =
+ optParamSetter.toSeq.flatMap { paramSetter =>
+ // there is a param setter with args that are supposed to
correspond to bound vars
+ val params = paramSetter.getParameters.toSeq
- val paramTypes = constructor.getParameterTypes.toSeq
- val paramVRDs = params.map { p =>
- lrd.vmap.getOrElse(
- p.getName,
- lrd.context.SDE(
- s"No layer DFDL variable named '$p.getName' was found in
namespace ${lrd.namespace}.",
- ),
- )
- }.toSeq
+ val paramTypes = paramSetter.getParameterTypes.toSeq
+ val pVRDs: Seq[VariableRuntimeData] = params.map { p =>
+ lrd.vmap.getOrElse(
+ p.getName,
+ lrd.context.SDE(
+ s"No layer DFDL variable named '$p.getName' was found in
namespace ${lrd.namespace}.",
+ ),
+ )
+ }.toSeq
- // Now we deal with the result getters and the corresponding vars
- //
- val allLayerVRDs = ListSet(lrd.vmap.toSeq.map(_._2): _*)
- //
- // verify only Int and String are allowed simple types for now.
- // The rest of the code beyond here doesn't care what the type is, but
we don't want to
- // create a bunch of test code for layer things that aren't needed.
- //
- allLayerVRDs.foreach { vrd =>
- lrd.context.schemaDefinitionUnless(
- vrd.primType == PrimType.String ||
- vrd.primType == PrimType.Int ||
- vrd.primType == PrimType.UnsignedShort,
- s"Layer variable ${vrd.globalQName.toQNameString} of type
${vrd.primType.dfdlType} must be of type xs:int, xs:unsignedShort, or
xs:string.",
- )
- }
+ // Now we deal with the result getters and the corresponding vars
+ //
+
+ //
+ // verify only Int and String are allowed simple types for now.
+ // The rest of the code beyond here doesn't care what the type is,
but we don't want to
+ // create a bunch of test code for layer things that aren't needed.
+ //
+ allLayerVRDs.foreach { vrd =>
+ lrd.context.schemaDefinitionUnless(
+ vrd.primType == PrimType.String ||
+ vrd.primType == PrimType.Int ||
+ vrd.primType == PrimType.UnsignedShort ||
+ vrd.primType == PrimType.UnsignedInt,
+ s"Layer variable ${vrd.globalQName.toQNameString} of type
${vrd.primType.dfdlType} must" +
+ s" be of type xs:int, xs:unsignedInt, xs:unsignedShort, or
xs:string.",
+ )
+ }
+ // at this point the number of vars and number of setter args match
+
+ val typePairs = (pVRDs.zip(paramTypes)).toSeq
+ typePairs.foreach { case (vrd, pt) =>
+ val vrdClass = PrimType.toJavaType(vrd.primType.dfdlType)
+ lrd.context.schemaDefinitionUnless(
+ compatibleTypes(vrdClass, pt),
+ s"""Layer setter argument ${vrd.globalQName.local} and the
corresponding
+ |Layer DFDL variable have differing types: ${pt.getName}
+ | and ${vrdClass.getName} respectively.""".stripMargin,
+ )
+ }
+ if (pVRDs.length != params.length) {
Review Comment:
I think this check will never fail and can be removed? Above you have `val
pVRDs = params.map { ... }` so the lengths will always be the same.
##########
daffodil-test/src/test/scala/org/apache/daffodil/runtime1/layers/CheckDigitLayer.scala:
##########
@@ -47,33 +47,37 @@ object CheckDigitLayer {
* - digitEncoding is a string which is the name of a character set encoding
which
* defines the character set of the digits.
* This must match the dfdl:encoding for the DFDL element declaration using
this layer.
- *
- * @param length the value of the length DFDL variable
- * @param params the value of the params DFDL variable
- * @param digitEncoding the value of the digitEncoding DFDL variable
*/
-class CheckDigitLayer(length: JInt, params: String, digitEncoding: String)
+class CheckDigitLayer
extends ChecksumLayer(
"checkDigit", // name of the layer, also happens to be the name of the
output variable.
CheckDigitLayer.ns,
- length,
) {
+ private var params: String = _
+ private var digitEncoding: String = _
+ private lazy val charset = Charset.forName(digitEncoding)
+ private lazy val isVerbose = params.toLowerCase.contains("verbose")
+
+ /**
+ * @param length the value of the length DFDL variable
+ * @param params the value of the params DFDL variable
+ * @param digitEncoding the value of the digitEncoding DFDL variable
+ */
+ def setLayerVariableParameters(length: Int, params: String, digitEncoding:
String): Unit = {
+ setLength(length)
+ this.params = params
+ this.digitEncoding = digitEncoding
+ }
+
/**
* Result DFDL variable value getter. This getter is called to obtain the
value for,
* and populate the DFDL variable named checkDigit, in the layer's target
namespace.
* @return the check digit value
*/
- def getDFDLResultVariable_checkDigit: JInt =
+ def getLayerVariableResult_checkDigit: JInt =
Review Comment:
Just curious if this must be a Jint or would a primitive Int work too? I
think it would work with the new changes? Should our examples use primitive int
since I imagine that what most Java devs would find more natural?
--
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]
