stevedlawrence commented on a change in pull request #651:
URL: https://github.com/apache/daffodil/pull/651#discussion_r722156628



##########
File path: 
daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -64,8 +64,6 @@ trait RawSequenceRuntimeValuedPropertiesMixin
 
 trait RawLayeringRuntimeValuedPropertiesMixin
   extends PropertyMixin {
-  protected final lazy val optionLayerTransformRaw = 
findPropertyOption("layerTransform", expressionAllowed = true)
-  protected final lazy val layerTransformRaw = 
requireProperty(optionLayerTransformRaw)
   protected final lazy val optionLayerEncodingRaw = 
findPropertyOption("layerEncoding", expressionAllowed = true)

Review comment:
       Unrelated to this change but how does the layer encoding property apply 
to these CRC layers that don't have an encoding? Are they required and just 
need to be ISO-8859-1 or something? Or should layer encoding be something like 
a required input variable to the layer, the way that some CRC require an input 
value?

##########
File path: 
daffodil-core/src/main/scala/org/apache/daffodil/layers/LayerCompilerRegistry.scala
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.layers
+
+
+import org.apache.daffodil.exceptions.ThrowsSDE
+
+import scala.collection.mutable
+
+/**
+ * Transformers have factories. This lets you find the transformer factory
+ * by the name obtained from dfdlx:layerTransform.
+ */
+object LayerCompilerRegistry {
+
+  private lazy val layerCompilerMap = new mutable.HashMap[String, 
LayerCompiler]
+
+  def register(layerCompiler: LayerCompiler): Unit = {
+    layerCompilerMap.put(layerCompiler.name, layerCompiler)
+  }
+  /**
+   * Given name, finds the factory for the transformer. SDE otherwise.
+   *
+   * The state is passed in order to provide diagnostic context if not found.
+   */
+  def find(name: String, context: ThrowsSDE): LayerCompiler = {
+    val maybeCompiler: Option[LayerCompiler] = layerCompilerMap.get(name)
+    if (maybeCompiler.isEmpty) {
+      //
+      // TODO: Daffodil-1927
+      // This is where we would try to dynamically load a layer transform 
compiler
+      // for the given layer name.
+      //
+      val choices = layerCompilerMap.keySet.mkString(", ")
+      context.SDE("The dfdlx:layerTransform '%s' was not found. Available 
choices are: %s", name, choices)
+    } else {
+      maybeCompiler.get
+    }
+  }

Review comment:
       Is the plan for plugability to replace this all with the `ServiceLoader` 
mechanism used by `UDF`s and `Validator`s?  

##########
File path: 
daffodil-runtime1/src/main/scala/org/apache/daffodil/layers/LayerCompileInfo.scala
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.layers
+
+import org.apache.daffodil.schema.annotation.props.gen.LayerLengthKind
+import org.apache.daffodil.schema.annotation.props.gen.LayerLengthUnits
+
+
+/**
+ * provides access to DFDL schema compile-time information about the layer 
properties
+ */
+trait LayerCompileInfo {
+  /**
+   * If defined, and the value is a compile-time constant then this will be 
Some(Some(Charset))
+   * If defined, and the value is non-constant then this will be Some(None)
+   * If undefined, the value is None
+   */
+  def optLayerJavaCharsetOptConstantValue: 
Option[Option[java.nio.charset.Charset]]
+  def optLayerLengthKind: Option[LayerLengthKind]
+  def optLayerLengthOptConstantValue: Option[Option[Long]]
+  def optLayerLengthUnits: Option[LayerLengthUnits]
+  def optLayerBoundaryMarkOptConstantValue: Option[Option[String]]
+  def schemaDefinitionError(message: String, args: Any*): Nothing
+  def schemaDefinitionWarning(message: String, args: Any*): Unit
+  final def schemaDefinitionUnless(test: Boolean, message: String, args: Any*) 
= if (!test) SDE(message, args: _*)

Review comment:
       I'm wondering if we shouldn't provide this function, since it will 
always evaluate message/args regardless of the result of test? Is it worth 
forcing users to use something like `if (test) compileInfo.SDE(...)`, or should 
we just trust them to do the right thing?
   
   Removing is also maybe nice in the sense that there is only one way to SDE, 
so it's maybe a bit simpler of an API?

##########
File path: 
daffodil-runtime1/src/main/scala/org/apache/daffodil/layers/LayerTransformer.scala
##########
@@ -123,19 +67,27 @@ object LayerTransformerFactory {
  * A layer transformer is created at runtime as part of a single parse/unparse 
call.
  * Hence, they can be stateful without causing thread-safety issues.
  */
-abstract class LayerTransformer() {
+abstract class LayerTransformer(layerName: String, layerRuntimeInfoArg: 
LayerRuntimeInfo) {
+
+  final def layerRuntimeInfo = 
layerRuntimeInfoArg.asInstanceOf[LayerRuntimeInfoImpl]

Review comment:
       This isn't safe, right? The `LayerTransformer` is created by the user, 
and so `LayerRuntimeInfo` doesn't necessarily need to be a 
`LayerRuntimeInfoImple`?

##########
File path: 
daffodil-test/src/test/scala/org/apache/daffodil/layers/CheckDigit.scala
##########
@@ -17,63 +17,23 @@
 
 package org.apache.daffodil.layers
 
-import org.apache.commons.io.IOUtils
-import org.apache.daffodil.schema.annotation.props.gen.LayerLengthKind
-import org.apache.daffodil.schema.annotation.props.gen.LayerLengthUnits
-import org.apache.daffodil.util.Maybe
-import org.apache.daffodil.processors.LayerLengthEv
-import org.apache.daffodil.processors.LayerBoundaryMarkEv
-import org.apache.daffodil.processors.LayerCharsetEv
-import org.apache.daffodil.processors.parsers.PState
-import org.apache.daffodil.processors.unparsers.UState
-
-import java.io._
-import org.apache.daffodil.dpath.NodeInfo.PrimType
-import org.apache.daffodil.exceptions.Assert
-import org.apache.daffodil.processors.CharsetEvBase
-import org.apache.daffodil.processors.SequenceRuntimeData
-import org.apache.daffodil.processors.SuspendableOperation
-import org.apache.daffodil.processors.charset.BitsCharsetJava
-import org.apache.daffodil.util.ByteBufferOutputStream
 import org.apache.daffodil.util.Logger
-import org.apache.daffodil.xml.NS
-import org.apache.daffodil.xml.RefQName
 
 import java.nio.ByteBuffer
-import java.nio.charset.Charset
-
 
-final class CheckDigitExplicit(
-  srd: SequenceRuntimeData,
-  charset: Charset,
-  layerLengthEv: LayerLengthEv)
-extends LayerTransformer() {
-  Assert.invariant(charset.newDecoder().maxCharsPerByte() == 1) // is a SBCS 
charset
-
-  private val checkDigitVRD = {
-    val varNamespace = NS("urn:org.apache.daffodil.layers.checkDigit")
-    val varQName = RefQName(Some("cd"), "checkDigit", 
varNamespace).toGlobalQName
-    val vrd = srd.variableMap.getVariableRuntimeData(varQName).getOrElse {
-      srd.SDE("Variable '%s' is not defined.", varQName.toExtendedSyntax)
-    }
-    srd.schemaDefinitionUnless(vrd.primType == PrimType.Int,
-      "Variable '%s' is not of type 'xs:int'.", varQName.toExtendedSyntax)
-    vrd
-  }
 
-  private val checkDigitParamsVRD = {
-    val varNamespace = NS("urn:org.apache.daffodil.layers.checkDigit")
-    val varQName = RefQName(Some("cd"), "checkDigitParams", 
varNamespace).toGlobalQName
-    val vrd = srd.variableMap.getVariableRuntimeData(varQName).getOrElse {
-      srd.SDE("Variable '%s' is not defined.", varQName.toExtendedSyntax)
-    }
-    srd.schemaDefinitionUnless(vrd.primType == PrimType.String,
-      "Variable '%s' is not of type 'xs:string'.", varQName.toExtendedSyntax)
-    vrd
-  }
+final class CheckDigitExplicit(name: String,
+  infoArg: LayerRuntimeInfo)
+extends ByteBufferExplicitLengthLayerTransform[Int](
+  infoArg,
+  name,
+  variablesNamespace = "urn:org.apache.daffodil.layers.checkDigit",
+  variablesPreferredNamespacePrefix = "cd",
+  localNamesAndTypesOfVariablesToRead = Seq(("checkDigitParams", "string")),

Review comment:
       In our UDF, we use `Class` for types. Thoughts on doing something 
similar?

##########
File path: 
daffodil-core/src/main/scala/org/apache/daffodil/layers/LayerCompilerInfoImpl.scala
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.layers
+
+import org.apache.daffodil.api.WarnID
+import org.apache.daffodil.dsom.SequenceTermBase
+import org.apache.daffodil.processors.LayerBoundaryMarkEv
+import org.apache.daffodil.processors.LayerCharsetEv
+import org.apache.daffodil.processors.LayerLengthEv
+import org.apache.daffodil.processors.charset.BitsCharsetJava
+import org.apache.daffodil.processors.charset.BitsCharsetNonByteSize
+import org.apache.daffodil.schema.annotation.props.gen.LayerLengthKind
+import org.apache.daffodil.schema.annotation.props.gen.LayerLengthUnits
+import org.apache.daffodil.util.Maybe
+
+
+/**
+ * Provides access to DFDL schema compile-time information about the layer 
properties.
+ *
+ * Allows reporting of schema definition errors and warnings at schema compile 
time.
+ */
+final class LayerCompileInfoImpl(sequence: SequenceTermBase,
+  maybeLayerCharsetEv: Maybe[LayerCharsetEv],
+  maybeLayerLengthKind: Maybe[LayerLengthKind],
+  maybeLayerLengthEv: Maybe[LayerLengthEv],
+  maybeLayerLengthUnits: Maybe[LayerLengthUnits],
+  maybeLayerBoundaryMarkEv: Maybe[LayerBoundaryMarkEv])
+extends LayerCompileInfo {
+  /**
+   * If defined, and the value is a compile-time constant then this will be 
Some(Some(Charset))
+   * If defined, and the value is non-constant or not a regular JVM charset, 
then this will be Some(None)
+   * If undefined, the value is None
+   */
+  override def optLayerJavaCharsetOptConstantValue: 
Option[Option[java.nio.charset.Charset]] = {
+    if (maybeLayerCharsetEv.isEmpty) None
+    else
+      maybeLayerCharsetEv.get.optConstant.map {
+        case java: BitsCharsetJava => Some(java.javaCharset)
+        case _: BitsCharsetNonByteSize => None

Review comment:
       Should this be an SDE if someone provides a layer encoding that isn't a 
standard java encoding? Is it worth adding a test to cover this?

##########
File path: 
daffodil-runtime1/src/main/scala/org/apache/daffodil/layers/Base64Transformer.scala
##########
@@ -17,38 +17,59 @@
 
 package org.apache.daffodil.layers
 
-import org.apache.daffodil.schema.annotation.props.gen.LayerLengthKind
-import org.apache.daffodil.schema.annotation.props.gen.LayerLengthUnits
-import org.apache.daffodil.util.Maybe
-import org.apache.daffodil.processors.LayerLengthEv
-import org.apache.daffodil.processors.LayerBoundaryMarkEv
-import org.apache.daffodil.processors.LayerCharsetEv
 import org.apache.daffodil.io.BoundaryMarkLimitingStream
-import org.apache.daffodil.processors.parsers.PState
-import org.apache.daffodil.processors.charset.BitsCharsetJava
-
-import java.nio.charset.Charset
-import org.apache.daffodil.processors.charset.BitsCharset
-import org.apache.daffodil.exceptions.Assert
-import org.apache.daffodil.processors.unparsers.UState
 import org.apache.daffodil.io.LayerBoundaryMarkInsertingJavaOutputStream
-import org.apache.daffodil.processors.SequenceRuntimeData
+import org.apache.daffodil.schema.annotation.props.gen.LayerLengthKind
+
+object Base64MIMELayerCompiler
+  extends LayerCompiler("base64_MIME") {
+
+  override def compileLayer(layerCompileInfo: LayerCompileInfo): 
Base64MIMETransformerFactory = {
+
+    layerCompileInfo.schemaDefinitionUnless(
+      scala.util.Properties.isJavaAtLeast("1.8"),
+      "Base64 layer support requires Java 8 (aka Java 1.8).")
+
+    layerCompileInfo.schemaDefinitionUnless(
+      layerCompileInfo.optLayerBoundaryMarkOptConstantValue.isDefined,
+      "Property dfdlx:layerBoundaryMark was not defined.")
+    layerCompileInfo.schemaDefinitionUnless(
+      layerCompileInfo.optLayerLengthKind.isEmpty ||
+        (layerCompileInfo.optLayerLengthKind.get eq 
LayerLengthKind.BoundaryMark),
+      "Only dfdlx:layerLengthKind 'boundaryMark' is supported, but '%s' was 
specified",
+      layerCompileInfo.optLayerLengthKind.get.toString)
+
+    layerCompileInfo.schemaDefinitionUnless(
+      layerCompileInfo.optLayerJavaCharsetOptConstantValue match {
+        case Some(Some(_)) => true
+        case _ => false
+      }, "Property dfdlx:layerEncoding must be defined.")
+
+    val xformer = new Base64MIMETransformerFactory(name, 
layerCompileInfo.layerRuntimeInfo)

Review comment:
       Our built-in layers all use the layerRuntimeInfo default implementation, 
but maybe they should each have their own implementation? It would avoid the 
`asInstanceOf` issue mentioned in another comment, since there essential is not 
a default implementation.

##########
File path: 
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/DFDL_part1_simpletypes.xsd
##########
@@ -697,13 +697,13 @@
     </xsd:union>
   </xsd:simpleType>
 
-  <xsd:simpleType name="LayerTransformType_Or_DFDLExpression">
+  <xsd:simpleType name="LayerTransformType">

Review comment:
       Should this whole type be moved to `dfdlx.xsd`?

##########
File path: 
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/LayeredSequenceUnparser.scala
##########
@@ -17,20 +17,22 @@
 
 package org.apache.daffodil.processors.unparsers
 
-import org.apache.daffodil.processors.LayerTransformerEv
+import org.apache.daffodil.layers.LayerRuntimeInfoImpl
+import org.apache.daffodil.layers.LayerTransformerFactory
 import org.apache.daffodil.processors.SequenceRuntimeData
 
 class LayeredSequenceUnparser(ctxt: SequenceRuntimeData,
-  layerTransformerEv: LayerTransformerEv,
+  layerTransformerFactory: LayerTransformerFactory,
   childUnparser: SequenceChildUnparser)
   extends OrderedUnseparatedSequenceUnparser(ctxt, Seq(childUnparser)) {
 
-  override lazy val runtimeDependencies = Vector(layerTransformerEv)
+  override lazy val runtimeDependencies =
+    
layerTransformerFactory.layerRuntimeInfo.asInstanceOf[LayerRuntimeInfoImpl].evaluatables.toVector

Review comment:
       I'm not sure this `asInstanceOf` is safe. If I'm following everything 
correctly, a user implements `LayerCompiler`, which has a `compilerLayer` 
function which returns a `LayerTransformerFactory`. This 
`LayerTransformerFactory` must have a `LayerRuntimeInfo` provided, but that is 
user implemented and so can be anything--it doesn't need to be a 
`LayerRuntimeInfoImpl` and it doesn't need to have any evaluatables.
   
   And I think a similar issue is in `LayerTransformer.scala`, which has a 
similar `asInstanceOf` where it assumes the `LayerRuntimeInfo` is a 
`LayerRuntimeInfoImpl`

##########
File path: 
daffodil-runtime1/src/main/scala/org/apache/daffodil/layers/LayerCompileInfo.scala
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.layers
+
+import org.apache.daffodil.schema.annotation.props.gen.LayerLengthKind
+import org.apache.daffodil.schema.annotation.props.gen.LayerLengthUnits
+
+
+/**
+ * provides access to DFDL schema compile-time information about the layer 
properties
+ */
+trait LayerCompileInfo {

Review comment:
       I'm wondering how Java friendly this trait is going to be? There's 
things like `Options`, `LayerLengthKind` and `LayerLengthUnits` objects, which 
I feel like is not always easy to use in Java. Is easy use in Java a goal for 
these changes, or do you think that might be too difficult to achieve?

##########
File path: 
daffodil-runtime1/src/main/scala/org/apache/daffodil/layers/LayerTransformer.scala
##########
@@ -282,4 +234,234 @@ case class LayerNotEnoughDataException(layerSRD: 
SequenceRuntimeData, state: PSt
   extends LayerException(layerSRD, state, One(cause), None, nBytesRequired) {
   override def isError = true
   override def modeName = "Parse"
-  }
\ No newline at end of file
+  }
+
+
+
+class LayerRuntimeInfoImpl(val srd: SequenceRuntimeData,
+  maybeLayerCharsetEv: Maybe[LayerCharsetEv],
+  maybeLayerLengthKind: Maybe[LayerLengthKind],
+  maybeLayerLengthEv: Maybe[LayerLengthEv],
+  maybeLayerLengthUnits: Maybe[LayerLengthUnits],
+  maybeLayerBoundaryMarkEv: Maybe[LayerBoundaryMarkEv])
+  extends LayerRuntimeInfo {
+
+  def evaluatables: Seq[Evaluatable[AnyRef]] =
+    maybeLayerCharsetEv.toScalaOption.toSeq ++
+      maybeLayerLengthEv.toScalaOption.toSeq ++
+      maybeLayerBoundaryMarkEv.toScalaOption.toSeq
+
+
+  def layerInfo(state: ParseOrUnparseState): LayerInfo =
+    new LayerInfoImpl(state, srd,
+      maybeLayerCharsetEv,
+      maybeLayerLengthKind,
+      maybeLayerLengthEv,
+      maybeLayerLengthUnits,
+      maybeLayerBoundaryMarkEv)
+}
+
+/**
+ * Allows access to all the layer properties, if defined, including
+ * evaluating expressions if the properties values are defined as expressions.
+ */
+sealed trait LayerInfo {
+  def optLayerCharset: Option[java.nio.charset.Charset]
+  def optLayerLengthKind: Option[LayerLengthKind]
+  def optLayerLength: Option[Long]
+  def optLayerLengthUnits: Option[LayerLengthUnits]
+  def optLayerBoundaryMark: Option[String]
+}
+
+class LayerInfoImpl(state: ParseOrUnparseState,
+  srd: SequenceRuntimeData,
+  maybeLayerCharsetEv: Maybe[LayerCharsetEv],
+  maybeLayerLengthKind: Maybe[LayerLengthKind],
+  maybeLayerLengthEv: Maybe[LayerLengthEv],
+  maybeLayerLengthUnits: Maybe[LayerLengthUnits],
+  maybeLayerBoundaryMarkEv: Maybe[LayerBoundaryMarkEv])
+extends LayerInfo {
+
+  override def optLayerCharset: Option[Charset] = 
maybeLayerCharsetEv.toScalaOption.map {
+    _.evaluate(state)
+  } match {
+    case Some(bitsCharsetJava: BitsCharsetJava) => 
Some(bitsCharsetJava.javaCharset)
+    case None => None
+    case Some(other) => None // only java charsets are supported.
+  }
+
+  override def optLayerLengthKind: Option[LayerLengthKind] = 
maybeLayerLengthKind.toScalaOption
+
+  override def optLayerLength: Option[Long] = 
maybeLayerLengthEv.toScalaOption.map{ _.evaluate(state) }
+
+  override def optLayerLengthUnits: Option[LayerLengthUnits] = 
maybeLayerLengthUnits.toScalaOption
+
+  override def optLayerBoundaryMark: Option[String] = 
maybeLayerBoundaryMarkEv.toScalaOption.map{ _.evaluate(state) }
+}
+
+
+abstract class ByteBufferExplicitLengthLayerTransform[T](
+  layerRuntimeInfoArg: LayerRuntimeInfo,
+  layerName: String,
+  variablesNamespace: String,
+  variablesPreferredNamespacePrefix: String,
+  localNamesAndTypesOfVariablesToRead: Seq[(String, String)],
+  localNameOfVariableToWrite: String,
+  outputVariableTypeLocalNameAsString: String)
+  extends LayerTransformer(layerName, layerRuntimeInfoArg) {

Review comment:
       Is the idea that this class is what custom CRC/checksum/etc. layers will 
want to implement? If so this needs comments on these parameters. I'm not 
really sure what all these variables mean or how they are intended to work.
   
   This also seems very specific to the use case of CRC/checksums, having 
input/output variables. Should this be renamed to make it clear this is 
intended only for those? Similar to the line folding transformer class?
   
   Also, not sure how Java friendly this is wit the type parameter and Seq of 
Tuples.

##########
File path: 
daffodil-runtime1/src/main/scala/org/apache/daffodil/layers/LayerTransformer.scala
##########
@@ -282,4 +234,234 @@ case class LayerNotEnoughDataException(layerSRD: 
SequenceRuntimeData, state: PSt
   extends LayerException(layerSRD, state, One(cause), None, nBytesRequired) {
   override def isError = true
   override def modeName = "Parse"
-  }
\ No newline at end of file
+  }
+
+
+
+class LayerRuntimeInfoImpl(val srd: SequenceRuntimeData,
+  maybeLayerCharsetEv: Maybe[LayerCharsetEv],
+  maybeLayerLengthKind: Maybe[LayerLengthKind],
+  maybeLayerLengthEv: Maybe[LayerLengthEv],
+  maybeLayerLengthUnits: Maybe[LayerLengthUnits],
+  maybeLayerBoundaryMarkEv: Maybe[LayerBoundaryMarkEv])
+  extends LayerRuntimeInfo {
+
+  def evaluatables: Seq[Evaluatable[AnyRef]] =
+    maybeLayerCharsetEv.toScalaOption.toSeq ++
+      maybeLayerLengthEv.toScalaOption.toSeq ++
+      maybeLayerBoundaryMarkEv.toScalaOption.toSeq
+
+
+  def layerInfo(state: ParseOrUnparseState): LayerInfo =
+    new LayerInfoImpl(state, srd,
+      maybeLayerCharsetEv,
+      maybeLayerLengthKind,
+      maybeLayerLengthEv,
+      maybeLayerLengthUnits,
+      maybeLayerBoundaryMarkEv)
+}
+
+/**
+ * Allows access to all the layer properties, if defined, including
+ * evaluating expressions if the properties values are defined as expressions.
+ */
+sealed trait LayerInfo {
+  def optLayerCharset: Option[java.nio.charset.Charset]
+  def optLayerLengthKind: Option[LayerLengthKind]
+  def optLayerLength: Option[Long]
+  def optLayerLengthUnits: Option[LayerLengthUnits]
+  def optLayerBoundaryMark: Option[String]
+}
+
+class LayerInfoImpl(state: ParseOrUnparseState,
+  srd: SequenceRuntimeData,
+  maybeLayerCharsetEv: Maybe[LayerCharsetEv],
+  maybeLayerLengthKind: Maybe[LayerLengthKind],
+  maybeLayerLengthEv: Maybe[LayerLengthEv],
+  maybeLayerLengthUnits: Maybe[LayerLengthUnits],
+  maybeLayerBoundaryMarkEv: Maybe[LayerBoundaryMarkEv])
+extends LayerInfo {
+
+  override def optLayerCharset: Option[Charset] = 
maybeLayerCharsetEv.toScalaOption.map {
+    _.evaluate(state)
+  } match {
+    case Some(bitsCharsetJava: BitsCharsetJava) => 
Some(bitsCharsetJava.javaCharset)
+    case None => None
+    case Some(other) => None // only java charsets are supported.
+  }
+
+  override def optLayerLengthKind: Option[LayerLengthKind] = 
maybeLayerLengthKind.toScalaOption
+
+  override def optLayerLength: Option[Long] = 
maybeLayerLengthEv.toScalaOption.map{ _.evaluate(state) }
+
+  override def optLayerLengthUnits: Option[LayerLengthUnits] = 
maybeLayerLengthUnits.toScalaOption
+
+  override def optLayerBoundaryMark: Option[String] = 
maybeLayerBoundaryMarkEv.toScalaOption.map{ _.evaluate(state) }
+}
+
+
+abstract class ByteBufferExplicitLengthLayerTransform[T](
+  layerRuntimeInfoArg: LayerRuntimeInfo,
+  layerName: String,
+  variablesNamespace: String,
+  variablesPreferredNamespacePrefix: String,
+  localNamesAndTypesOfVariablesToRead: Seq[(String, String)],
+  localNameOfVariableToWrite: String,
+  outputVariableTypeLocalNameAsString: String)
+  extends LayerTransformer(layerName, layerRuntimeInfoArg) {
+
+  /**
+   * Override to specify the length exactly.
+   */
+  protected def explicitLength() = -1L

Review comment:
       Should we remove this default implementation? Seems like -1 wouldn't 
never make sense and a user must implement this function.

##########
File path: 
daffodil-runtime1/src/main/scala/org/apache/daffodil/layers/LayerTransformer.scala
##########
@@ -282,4 +234,234 @@ case class LayerNotEnoughDataException(layerSRD: 
SequenceRuntimeData, state: PSt
   extends LayerException(layerSRD, state, One(cause), None, nBytesRequired) {
   override def isError = true
   override def modeName = "Parse"
-  }
\ No newline at end of file
+  }
+
+
+
+class LayerRuntimeInfoImpl(val srd: SequenceRuntimeData,
+  maybeLayerCharsetEv: Maybe[LayerCharsetEv],
+  maybeLayerLengthKind: Maybe[LayerLengthKind],
+  maybeLayerLengthEv: Maybe[LayerLengthEv],
+  maybeLayerLengthUnits: Maybe[LayerLengthUnits],
+  maybeLayerBoundaryMarkEv: Maybe[LayerBoundaryMarkEv])
+  extends LayerRuntimeInfo {
+
+  def evaluatables: Seq[Evaluatable[AnyRef]] =
+    maybeLayerCharsetEv.toScalaOption.toSeq ++
+      maybeLayerLengthEv.toScalaOption.toSeq ++
+      maybeLayerBoundaryMarkEv.toScalaOption.toSeq
+
+
+  def layerInfo(state: ParseOrUnparseState): LayerInfo =
+    new LayerInfoImpl(state, srd,
+      maybeLayerCharsetEv,
+      maybeLayerLengthKind,
+      maybeLayerLengthEv,
+      maybeLayerLengthUnits,
+      maybeLayerBoundaryMarkEv)
+}
+
+/**
+ * Allows access to all the layer properties, if defined, including
+ * evaluating expressions if the properties values are defined as expressions.
+ */
+sealed trait LayerInfo {

Review comment:
       It's not totally clear to me the difference between `LayerCompileInfo`, 
`LayerRuntimeInfo`, and `LayerInfo`. Is this correct:
   
   `LayerCompileInfo` contains information that can be used to determine what 
`LayerFactory` is created, but should not be serialized?
   
   `LayerRuntimeInfo` can contain whatever information the Layer needs to do 
it's thing. The default implemetation is basically the same as the 
`LayerCompileInfo`, but this need not be the case. It is serialized along with 
the `LayerTransformFactory` and can be used to determine how a 
`LayerTransformer` is created at parse/unparse time when `newInstance` is 
called.
   
   `LayerInfo` only makes sense when `LayerRuntimeInfo` is the specific `Impl` 
kind, and contains the result of expressions that couldn't be evaluated at 
compile time. The results of these expression cannot be used to determine the 
parameters of the LayerTransformer--it only can affect the wrapLimitingStream 
function.
   
   Is this accurate? If so, it's not clear to me why we need these three 
things. Could we have `LayerCompileInfo` as is, and it is only used to create 
the `LayerTransfomerFactory`. Then at parse time, we create LayerInfo (suggest 
renaming to LayerRuntimeInfo, and pass that into the `newInstance`. Now that 
instance has all the evaluated runtime values, and can create the right 
instance, and use it for things like wrapping, etc. This way the concept of a 
Runtime thing being stored in the serialized Factory doesn't exist. Runtime 
inforation is only available for Transformer.




-- 
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]


Reply via email to