stevedlawrence commented on a change in pull request #394: URL: https://github.com/apache/incubator-daffodil/pull/394#discussion_r453795846
########## File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/walker/AbstractDSOMWalker.scala ########## @@ -0,0 +1,163 @@ +/* + * 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.dsom.walker + +import org.apache.daffodil.dpath.NodeInfo.PrimType + +trait CommonContextView { + def namespaces: scala.xml.NamespaceBinding +} + +trait TermView extends CommonContextView { + def isArray: Boolean + def isOptional: Boolean + def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { + walker.onTermBegin(this) + walker.onTermEnd(this) + } +} + +trait SimpleTypeView { + def primType: PrimType +} +trait ComplexTypeView { + def group: ModelGroupView +} + +trait ModelGroupView extends TermView { + def groupMembers: Seq[TermView] + override def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { + walker.onTermBegin(this) + this.groupMembers.foreach(_.walkDSOM(walker)) + walker.onTermEnd(this) + } +} + +trait ChoiceView extends ModelGroupView +trait SequenceView extends ModelGroupView +trait GroupRefView extends ModelGroupView { + def isHidden: Boolean + override def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = if (!isHidden) super.walkDSOM(walker) +} + +trait ElementDeclView extends CommonContextView { + def isSimpleType: Boolean + def isComplexType: Boolean + def simpleType: SimpleTypeView + def complexType: ComplexTypeView + def hasDefaultValue: Boolean +} + +trait ElementBaseView extends ElementDeclView with TermView { + def name: String + override def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { + walker.onTermBegin(this) + if (isComplexType) { + walker.onTypeBegin(Right(complexType)) + complexType.group.walkDSOM(walker) + walker.onTypeBegin(Right(complexType)) + } else { + walker.onTypeBegin(Left(simpleType)) + walker.onTypeBegin(Left(simpleType)) + } + walker.onTermEnd(this) + } + +} + +trait RootView extends ElementBaseView + +/** + * A class designed to walk the internal representation of a DFDL Schema File. + * + * There are 2 main event handlers an implementing class has to worry about: one for Terms, and + * another for Types. These are called as the DSOM is walked. + * + * Though recursion is used here to define the walk, it is not advised to use recursion between + * these event handlers. Instead, consider a stack-like structure, as the DFDL Schema structure + * as well as the recursive method call structure can be represented by trees. + * @tparam T the return type of walkDSOMSchema and onWalkEnd. This should be a class used to + * represent the structure of the Schema. You could always make it Unit if you don't + * want these methods to return anything. + */ +abstract class AbstractDSOMWalker[T] { + + /** Review comment: Regarding the *View stuff, this seems reasonable to me. One concern is that we probably want this to be a public stable API, that works in both Java and Scala. I *think* Scala traits get turned into interfaces that a Java class can access, but I'm not 100% sure. If we generate JavaDocs, which we probably want to do, I'm also not sure if the Scala traits turn into something useful that makes sense for a java API. Note that we use the genjavadocs plugin to generate javadocs for daffodil-japi. I'm wondering if we want to do something similar? So maybe we want to move all the walker related things that are part of the public API (i.e. everything in the dsom.walker package) to a daffodil-dsom-walker subproject? Then we can generate JavaDocs for that just like with do with the Daffodil JAPI. Related, I'm looking into [DAFFODIL-2368](https://issues.apache.org/jira/browse/DAFFODIL-2368), which is related to how we can merge docs from multiple subprojects. I think looking at the Javadocs that results from this would be a clear way to view what parts of this API are public and what aren't. ########## File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/walker/AbstractDSOMWalker.scala ########## @@ -0,0 +1,163 @@ +/* + * 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.dsom.walker + +import org.apache.daffodil.dpath.NodeInfo.PrimType + +trait CommonContextView { + def namespaces: scala.xml.NamespaceBinding +} + +trait TermView extends CommonContextView { + def isArray: Boolean + def isOptional: Boolean + def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { + walker.onTermBegin(this) + walker.onTermEnd(this) + } +} + +trait SimpleTypeView { + def primType: PrimType +} +trait ComplexTypeView { + def group: ModelGroupView +} + +trait ModelGroupView extends TermView { + def groupMembers: Seq[TermView] + override def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { + walker.onTermBegin(this) + this.groupMembers.foreach(_.walkDSOM(walker)) + walker.onTermEnd(this) + } +} + +trait ChoiceView extends ModelGroupView +trait SequenceView extends ModelGroupView +trait GroupRefView extends ModelGroupView { + def isHidden: Boolean + override def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = if (!isHidden) super.walkDSOM(walker) +} + +trait ElementDeclView extends CommonContextView { + def isSimpleType: Boolean + def isComplexType: Boolean + def simpleType: SimpleTypeView + def complexType: ComplexTypeView + def hasDefaultValue: Boolean +} + +trait ElementBaseView extends ElementDeclView with TermView { + def name: String + override def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { + walker.onTermBegin(this) + if (isComplexType) { + walker.onTypeBegin(Right(complexType)) + complexType.group.walkDSOM(walker) + walker.onTypeBegin(Right(complexType)) + } else { + walker.onTypeBegin(Left(simpleType)) + walker.onTypeBegin(Left(simpleType)) + } + walker.onTermEnd(this) + } + +} + +trait RootView extends ElementBaseView + +/** + * A class designed to walk the internal representation of a DFDL Schema File. + * + * There are 2 main event handlers an implementing class has to worry about: one for Terms, and + * another for Types. These are called as the DSOM is walked. + * + * Though recursion is used here to define the walk, it is not advised to use recursion between + * these event handlers. Instead, consider a stack-like structure, as the DFDL Schema structure + * as well as the recursive method call structure can be represented by trees. + * @tparam T the return type of walkDSOMSchema and onWalkEnd. This should be a class used to + * represent the structure of the Schema. You could always make it Unit if you don't + * want these methods to return anything. + */ +abstract class AbstractDSOMWalker[T] { + + /** + * Method to be called on the beginning of the traversal. It is recommended to add some + * sort of wrapper element to a stack if you're doing a typical stack-based traversal. + * + * @param root the root element of the DFDL Schema + */ + protected def onWalkBegin(root: RootView): Unit + + /** + * Method to be called when the traversal concludes. It is recommended to put any post-processing + * and anything to tidy up the stack or the result here. + * + * @param root the root element of the DFDL Schema + * @return the result of the traversal + */ + protected def onWalkEnd(root: RootView): T + + /** + * Method to be called whenever any element that is a Term is encountered. + * This applies to Sequence, Choice, GroupRef, ElementBase, etc. + * + * It is highly recommended that, when implementing this method, you pattern match + * some of these different sub-types at some point to handle each accordingly. + * + * @param termElement the term element + */ + def onTermBegin(termElement: TermView): Unit + + /** + * Method to be called when a Term element has finished processing + * + * @param termElement the term element + */ + def onTermEnd(termElement: TermView): Unit + + /** + * Method to be called when either a Simple or Complex Type element has been encountered. + * This is just the element for <simpleType> or <complexType>, so no implementation is necessary if you + * do not care about these wrapper portion; its children will be called automatically. + * + * @param typeElement either a Simple or Complex type + */ + def onTypeBegin(typeElement: Either[SimpleTypeView, ComplexTypeView]): Unit Review comment: Either's are a scala construct, which means this would likely be difficult, or at least uncomfortable, for a Java user to use. Perhaps we have a shared parent of SimpleTypeView and ComplexTypeView and require the user to use isntanceOf, similar to the onTerm methods? ########## File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/walker/AbstractDSOMWalker.scala ########## @@ -0,0 +1,163 @@ +/* + * 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.dsom.walker + +import org.apache.daffodil.dpath.NodeInfo.PrimType + +trait CommonContextView { + def namespaces: scala.xml.NamespaceBinding +} + +trait TermView extends CommonContextView { + def isArray: Boolean + def isOptional: Boolean + def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { + walker.onTermBegin(this) + walker.onTermEnd(this) + } +} + +trait SimpleTypeView { + def primType: PrimType +} +trait ComplexTypeView { + def group: ModelGroupView +} + +trait ModelGroupView extends TermView { + def groupMembers: Seq[TermView] + override def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { + walker.onTermBegin(this) + this.groupMembers.foreach(_.walkDSOM(walker)) + walker.onTermEnd(this) + } +} + +trait ChoiceView extends ModelGroupView +trait SequenceView extends ModelGroupView +trait GroupRefView extends ModelGroupView { + def isHidden: Boolean + override def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = if (!isHidden) super.walkDSOM(walker) +} + +trait ElementDeclView extends CommonContextView { + def isSimpleType: Boolean + def isComplexType: Boolean + def simpleType: SimpleTypeView + def complexType: ComplexTypeView + def hasDefaultValue: Boolean +} + +trait ElementBaseView extends ElementDeclView with TermView { + def name: String + override def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { + walker.onTermBegin(this) + if (isComplexType) { + walker.onTypeBegin(Right(complexType)) + complexType.group.walkDSOM(walker) + walker.onTypeBegin(Right(complexType)) + } else { + walker.onTypeBegin(Left(simpleType)) + walker.onTypeBegin(Left(simpleType)) + } + walker.onTermEnd(this) + } + +} + +trait RootView extends ElementBaseView + +/** + * A class designed to walk the internal representation of a DFDL Schema File. + * + * There are 2 main event handlers an implementing class has to worry about: one for Terms, and + * another for Types. These are called as the DSOM is walked. + * + * Though recursion is used here to define the walk, it is not advised to use recursion between + * these event handlers. Instead, consider a stack-like structure, as the DFDL Schema structure + * as well as the recursive method call structure can be represented by trees. + * @tparam T the return type of walkDSOMSchema and onWalkEnd. This should be a class used to + * represent the structure of the Schema. You could always make it Unit if you don't + * want these methods to return anything. + */ +abstract class AbstractDSOMWalker[T] { Review comment: I looks like the type parameter is only used for onWalkEnd. My conern is that this type parameter might not translate to a Java user very well. I'm not sure how Scala converts these to byte code. Perhaps it just becomes a Java generic and works fine? But considering it's only used for the onWalkEnd method, and thuse onWalkFromRoot, I'm wondering if adds much. If a implementation of AbstractDSOMWalker needs a return value of onWalkEnd, they can implement onWalkEnd to store state and access that state after onWalkFromRoot returns? It's maybe a little uglier, but maybe avoids type parameter issues with a Java user of this API? ########## File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/walker/AbstractDSOMWalker.scala ########## @@ -0,0 +1,163 @@ +/* + * 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.dsom.walker + +import org.apache.daffodil.dpath.NodeInfo.PrimType + +trait CommonContextView { + def namespaces: scala.xml.NamespaceBinding +} + +trait TermView extends CommonContextView { + def isArray: Boolean + def isOptional: Boolean + def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { + walker.onTermBegin(this) + walker.onTermEnd(this) + } +} + +trait SimpleTypeView { + def primType: PrimType +} +trait ComplexTypeView { + def group: ModelGroupView +} + +trait ModelGroupView extends TermView { + def groupMembers: Seq[TermView] + override def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { + walker.onTermBegin(this) + this.groupMembers.foreach(_.walkDSOM(walker)) + walker.onTermEnd(this) + } +} + +trait ChoiceView extends ModelGroupView +trait SequenceView extends ModelGroupView +trait GroupRefView extends ModelGroupView { + def isHidden: Boolean + override def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = if (!isHidden) super.walkDSOM(walker) +} + +trait ElementDeclView extends CommonContextView { + def isSimpleType: Boolean + def isComplexType: Boolean + def simpleType: SimpleTypeView + def complexType: ComplexTypeView + def hasDefaultValue: Boolean +} + +trait ElementBaseView extends ElementDeclView with TermView { + def name: String + override def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { + walker.onTermBegin(this) + if (isComplexType) { + walker.onTypeBegin(Right(complexType)) + complexType.group.walkDSOM(walker) + walker.onTypeBegin(Right(complexType)) + } else { + walker.onTypeBegin(Left(simpleType)) + walker.onTypeBegin(Left(simpleType)) + } + walker.onTermEnd(this) + } + +} + +trait RootView extends ElementBaseView + +/** + * A class designed to walk the internal representation of a DFDL Schema File. + * + * There are 2 main event handlers an implementing class has to worry about: one for Terms, and + * another for Types. These are called as the DSOM is walked. + * + * Though recursion is used here to define the walk, it is not advised to use recursion between + * these event handlers. Instead, consider a stack-like structure, as the DFDL Schema structure + * as well as the recursive method call structure can be represented by trees. + * @tparam T the return type of walkDSOMSchema and onWalkEnd. This should be a class used to + * represent the structure of the Schema. You could always make it Unit if you don't + * want these methods to return anything. + */ +abstract class AbstractDSOMWalker[T] { + + /** + * Method to be called on the beginning of the traversal. It is recommended to add some + * sort of wrapper element to a stack if you're doing a typical stack-based traversal. + * + * @param root the root element of the DFDL Schema + */ + protected def onWalkBegin(root: RootView): Unit + + /** + * Method to be called when the traversal concludes. It is recommended to put any post-processing + * and anything to tidy up the stack or the result here. + * + * @param root the root element of the DFDL Schema + * @return the result of the traversal + */ + protected def onWalkEnd(root: RootView): T + + /** + * Method to be called whenever any element that is a Term is encountered. + * This applies to Sequence, Choice, GroupRef, ElementBase, etc. + * + * It is highly recommended that, when implementing this method, you pattern match + * some of these different sub-types at some point to handle each accordingly. + * + * @param termElement the term element + */ + def onTermBegin(termElement: TermView): Unit Review comment: One concern with Term is that it potentially requires the user to pattern match to figure out what the term is and what to do with it. This is very natural in Scala, but in Java I think this is going to be a much of if-statements with various ``instanceOf``'s and type casting. Maybe not a big deal? I'm not sure. ########## File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/Term.scala ########## @@ -18,16 +18,19 @@ package org.apache.daffodil.dsom import java.util.UUID + import org.apache.daffodil.exceptions.Assert import org.apache.daffodil.grammar.TermGrammarMixin import org.apache.daffodil.schema.annotation.props.gen.YesNo -import java.lang.{ Integer => JInt } +import java.lang.{Integer => JInt} Review comment: The style we standardized on is to use spaces, though I think that's not the default in Eclipse. There should be a setting to change this though. ########## File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/walker/AbstractDSOMWalker.scala ########## @@ -0,0 +1,163 @@ +/* + * 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.dsom.walker + +import org.apache.daffodil.dpath.NodeInfo.PrimType + +trait CommonContextView { + def namespaces: scala.xml.NamespaceBinding +} + +trait TermView extends CommonContextView { + def isArray: Boolean + def isOptional: Boolean + def walkDSOM[T](walker: AbstractDSOMWalker[T]): Unit = { Review comment: Do we want these walkDSOM function to be protected? Is there a reason that user might want to call walkDSOM directrly? We can also make them public in the future if needed, but it's much harder to switch to make them protected if they are already public. ########## File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/SimpleTypes.scala ########## @@ -53,9 +54,9 @@ trait TypeBase { trait NonPrimTypeMixin sealed trait SimpleTypeBase extends TypeBase - with HasOptRepTypeMixin { + with HasOptRepTypeMixin with SimpleTypeView { - def primType: PrimType + override def primType: PrimType Review comment: Giving direct access to PrimType is potentially problematic from a backwards compatibility perspective. This is something that has changed fairly frequently. I wonder if we also want a walker specific view of PrimType's as well? ########## File path: daffodil-core/src/main/scala/org/apache/daffodil/compiler/Compiler.scala ########## @@ -99,6 +99,8 @@ final class ProcessorFactory private( new SchemaSet(optRootSpec, schemaSource, validateDFDLSchemas, checkAllTopLevel, tunables, compilerExternalVarSettings)) + def walkDSOM[T](walker: AbstractDSOMWalker[T]): T = walker.walkFromRoot(sset.root) Review comment: Normally one would call ``processorFactory.onPath(..)``, which asserts that there are no errors and I think can result in finishing some lazy compilation? Two questions, more directed at Mike who I think knows more about the internal compialtion steps than me: 1. Should we also assert here that there are no errors? 2. Do we need to also do something to account for lazy compilation, such as ExecutionMode.usingCompilerMode? Is it possible for an SDE to get hit while walking and performing lazy compliation, or should all that be finished by the time onPath is called? ########## File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ModelGroup.scala ########## @@ -177,7 +180,7 @@ abstract class ModelGroup(index: Int) * * This also depends on groupMembersDef overrides all being lazy val. */ - final lazy val groupMembers: Seq[Term] = + override final lazy val groupMembers: Seq[Term] = Review comment: Giving access to groupMembers probably makes it so the whole walker stuff isn't really necessary. Someone could walk the DSOM entirely with their own implementation rather than relying on all the callbacks and having to store state. Not a big deal I guess, and maybe a benefit? ---------------------------------------------------------------- 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: [email protected]
