[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r397986310
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/GrammarTerm.scala
##
@@ -94,16 +94,19 @@ abstract class Gram(contextArg: SchemaComponent)
* Note: This should not evaluate the argument unless it has to.
*/
def ~(qq: => Gram) = {
-lazy val q = qq.deref
+val q = qq.deref
Review comment:
The comment above says this shouldn't evaluate qq unless it needs to. But
this looks like it will always evaluate it now. But maybe it always did that
regardless?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r397978972
##
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/GroupRef.scala
##
@@ -129,18 +131,15 @@ final class ChoiceGroupRef(
refXML: Node,
refLexicalParent: SchemaComponent,
positionArg: Int,
- isHiddenArg: Boolean)
+ override val isHidden: Boolean)
extends ChoiceTermBase(refXML, Option(refLexicalParent), positionArg)
with GroupRef {
requiredEvaluationsIfActivated(groupDef)
private lazy val globalGroupDef = globalGroupDefArg // once only
- override def isHidden = isHiddenArg
- override def isHiddenGroupRef = isHiddenArg
-
- override lazy val groupDef = globalGroupDef
+ override lazy val groupDef = LV('cgrGroupDef){ globalGroupDef }.value
Review comment:
Is it important for the symbol used in the LV match the variable name, maybe
for debugging or diagnostic purposes?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r397987021
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/GrammarTerm.scala
##
@@ -94,16 +94,19 @@ abstract class Gram(contextArg: SchemaComponent)
* Note: This should not evaluate the argument unless it has to.
*/
def ~(qq: => Gram) = {
-lazy val q = qq.deref
+val q = qq.deref
val self = this.deref
val res =
if (self.isEmpty) {
-if (q.isEmpty)
+if (q.isEmpty) {
EmptyGram
-else q
- } else if (q.isEmpty) self
- else {
+} else {
+ q
+}
+ } else if (q.isEmpty) {
+self
Review comment:
I think this is the only change here? I'm not sure I understandt this
change. What issue does this fix?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r392950118 ## File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/SchemaComponent.scala ## @@ -243,12 +227,12 @@ trait SchemaComponent case ct: ComplexTypeBase => "ct" case st: SimpleTypeDefBase => "st=" + st.namedQName.toQNameString case st: SimpleTypeBase => "st=" + st.primType.globalQName.toQNameString -case cgr: ChoiceGroupRef => "cgr" + (if (cgr.position > 1) cgr.position else "") + "=" + cgr.groupDef.namedQName.toQNameString +case cgr: ChoiceGroupRef => "cgr" + (if (cgr.isHidden) "h" else "") + (if (cgr.position > 1) cgr.position else "") + "=" + cgr.groupDef.namedQName.toQNameString Review comment: I noticed SequenceGroupRef below doesn't use ``toQNameString``. Is that a bug in SequenceGroupRef, or is there some fundamental differece between choices and groups? Maybe the toString function in namedQName just calls toQNameString and it's redundant? 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r392961023
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/ChoiceCombinator.scala
##
@@ -217,46 +217,58 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
}
override lazy val unparser: Unparser = {
-if (!ch.isHidden) {
- val eventRDMap = ch.choiceBranchMap
- val eventUnparserMap = eventRDMap.flatMap {
-case (cbe, rd) =>
- // if we don't find a matching RD for a term that's probably
- // because the term is an empty sequence or empty choice (which do
happen
- // and we even have tests for them). Since those can never be chosen
by
- // means of an element event, they don't appear in the map.
- val altGram = alternatives.find { alt =>
-val crd = alt.context.runtimeData
-val found = crd =:= rd
-found
+/*
+ * Since it's impossible to know the hiddenness for terms at this level
(unless
+ * they're a hiddenGroupRef), we always attempt to find a defaultable
unparser.
+ * If we find one, and at runtime, we're in a hidden state, we can use
that. If we
+ * are not in a hidden state, it doesn't get used. If we don't find one,
and we encounter
+ * a hidden group ref here, we know we'll run into that issue at runtime,
so we SDE.
Review comment:
I think the last sentence of this comment isn't correct. Looks like the code
below is correct in that we can always determine staticaly if we are a hidden
group that doesn't contains a defaultable branch.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r392943983
##
File path:
daffodil-cli/src/it/scala/org/apache/daffodil/debugger/TestCLIDebugger.scala
##
@@ -900,10 +900,14 @@ class TestCLIdebugger {
}
@Test def test_CLI_Debugger_InfoHidden_1() {
-val schemaFile =
Util.daffodilPath("daffodil-test/src/test/resources/org/apache/daffodil/section14/sequence_groups/SequencesWithHiddenRefs.dfdl.xsd")
-val inputFile =
Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/input/test_isHidden1.txt")
-val (testSchemaFile, testInputFile) = if (Util.isWindows)
(Util.cmdConvert(schemaFile), Util.cmdConvert(inputFile)) else (schemaFile,
inputFile)
-
+val schemaFile = Util.daffodilPath(
Review comment:
Agreed, I thought we had a bug for this already, but I can't find it. I've
created [DAFFODIL-2292](https://issues.apache.org/jira/browse/DAFFODIL-2292) to
track this issue. We probably shouldn't attempt ot fix it as part of this bug.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r392967071
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##
@@ -1497,6 +1482,17 @@ class InteractiveDebugger(runner:
InteractiveDebuggerRunner, eCompilers: Express
}
}
+ object InfoHidden extends DebugCommand with DebugCommandValidateZeroArgs
{
+val name = "hidden"
+override lazy val short = "hid"
+val desc = "display whether or not we're within a hidden state"
Review comment:
Doesn't look like this was resolved.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r392956719
##
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/Term.scala
##
@@ -612,46 +612,32 @@ trait Term
}.value
/*
- * This function returns at list of simple elements that are descendents of
- * this term that are not defaultable or OVC. This is a requirement for terms
- * inside a hidden group. Note that there is an exception for choices, in
- * which only a single branch be all defaultable or OVC. If any elements in a
- * hidden group are not defaultable or OVC, then it is an SDE. This function
- * assumes it is only called on elements inside of a hidden group.
+ * This function returns a boolean if the values of the term can be figured
+ * out during unparsing or if they don't need to appear in the infoset at
all.
*
- * Note that this currently only requires OVC since default's aren't
- * implemented. This function may need to change when we support defaults.
+ * Usually this means the term/its descendants have a default value (i.e
defaultable),
+ * have defined dfdl:outputValueCalc, or are optional (minOccurs=0)
+ *
+ * Note that this currently only requires OVC and Optionally since defaults
+ * aren't fully implemented everywhere. This function may need to change when
+ * defaults are fully implemented.
*/
- lazy val childrenInHiddenGroupNotDefaultableOrOVC: Seq[ElementBase] = {
-// this should only be called on hidden elements
-val isH = isHidden
-Assert.invariant(isH)
-
+ lazy val isFullyDefaultableOrOVC: Boolean = {
Review comment:
I"m wondering if we could come up with a different name since this also
takes into account optionality but the name does not imply that? From your
first sentence in the comment, maybe something like ``canUnparseIfHidden``.
That way if the logic becomes more complex due to some new feature, we don't
necessarily have to change the name.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r392961686
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/ChoiceCombinator.scala
##
@@ -217,46 +217,58 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
}
override lazy val unparser: Unparser = {
-if (!ch.isHidden) {
- val eventRDMap = ch.choiceBranchMap
- val eventUnparserMap = eventRDMap.flatMap {
-case (cbe, rd) =>
- // if we don't find a matching RD for a term that's probably
- // because the term is an empty sequence or empty choice (which do
happen
- // and we even have tests for them). Since those can never be chosen
by
- // means of an element event, they don't appear in the map.
- val altGram = alternatives.find { alt =>
-val crd = alt.context.runtimeData
-val found = crd =:= rd
-found
+/*
+ * Since it's impossible to know the hiddenness for terms at this level
(unless
+ * they're a hiddenGroupRef), we always attempt to find a defaultable
unparser.
+ * If we find one, and at runtime, we're in a hidden state, we can use
that. If we
+ * are not in a hidden state, it doesn't get used. If we don't find one,
and we encounter
+ * a hidden group ref here, we know we'll run into that issue at runtime,
so we SDE.
+ */
+val optDefaultableBranchUnparser = {
Review comment:
Wondering if we could also change this name ,since
``defaultableBranchUnparser``doesn't really imply OVC-ability, or optionality.
Maybsome something like ``unparseableBranchIfHidden``, or something similar to
whatever name we come up with for isFullyDefaultableOrOVC?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r392953986
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/dsom/SequenceGroup.scala
##
@@ -211,11 +211,16 @@ abstract class SequenceGroupTermBase(
}
lazy val checkHiddenSequenceIsDefaultableOrOVC: Unit = {
-if (this.isHidden) {
Review comment:
I'm wondering if we can completely remove this function and replace it with
something that is similar to what you've added in ChoiceCombinator to cause an
SDE, but in SequenceCombinator?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r392946570
##
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/GroupRef.scala
##
@@ -30,6 +30,11 @@ trait GroupRef { self: ModelGroup =>
final def referredToComponent = groupDef
+ /**
+ * Override in sequenceGroupRef and choiceGroupRef for hidden groups
+ */
+ def isHidden = false
Review comment:
Not a big deal at all, but more from a style perspective, I'm wondering if
it makes sense to not provide a default value for this, so it's just ``def
isHidden: Boolean``. The only two things that mixin ``GroupRef``s are
``SequenceGroupRef`` and ``ChoiceGroupRef`` and they both overridden this
already. And if there's ever somthing else that mixes in ``GroupRef``, they'll
be forced to think about how ``isHidden`` should be set or they'll get a
compile error.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r392964299
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/ChoiceCombinator.scala
##
@@ -217,46 +217,58 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
}
override lazy val unparser: Unparser = {
-if (!ch.isHidden) {
- val eventRDMap = ch.choiceBranchMap
- val eventUnparserMap = eventRDMap.flatMap {
-case (cbe, rd) =>
- // if we don't find a matching RD for a term that's probably
- // because the term is an empty sequence or empty choice (which do
happen
- // and we even have tests for them). Since those can never be chosen
by
- // means of an element event, they don't appear in the map.
- val altGram = alternatives.find { alt =>
-val crd = alt.context.runtimeData
-val found = crd =:= rd
-found
+/*
+ * Since it's impossible to know the hiddenness for terms at this level
(unless
+ * they're a hiddenGroupRef), we always attempt to find a defaultable
unparser.
+ * If we find one, and at runtime, we're in a hidden state, we can use
that. If we
+ * are not in a hidden state, it doesn't get used. If we don't find one,
and we encounter
+ * a hidden group ref here, we know we'll run into that issue at runtime,
so we SDE.
+ */
+val optDefaultableBranchUnparser = {
+ val defaultableBranches = ch.groupMembers.find {
_.isFullyDefaultableOrOVC }
+ if (defaultableBranches.nonEmpty) {
+val defaultableBranchRD = defaultableBranches.get.runtimeData
+
+// Choices inside a hidden group ref are slightly different because we
+// will never see events for any of the branches. Instead, we will just
+// always pick the branch in which every thing is defaultble or OVC.
+val defaultableBranchUnparser =
alternatives.find(_.context.runtimeData =:= defaultableBranchRD).get.unparser
+Some(defaultableBranchUnparser)
+ } else {
+ch match {
+ case cgr: GroupRef if cgr.isHidden => {
+/*
+ * This is a requirement for terms inside a hidden group. If we're
a hidden group ref, we know
+ * our descendants need this requirements, so it's a SDE if they
don't meet it.
+ */
+cgr.SDE("Element(s) of hidden group must be fully defaultable or
define dfdl:outputValueCalc:\n%s",
Review comment:
This is a choice, so only one branch needs to be unparsable if hidden. The
error message sortof implies all need to be unparsable. Also, this comment
doesn't mention optionality, but maybe that's not necessary, since if someone
hits this the solution is probabl to add a default or OVC.
Also, if we got here, that means no branches were unparseable, so we can
just print them all, e.g. ``ch.groupMembers.mkString("\n")``
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r390411621
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/ProcessorStateBases.scala
##
@@ -480,6 +480,17 @@ abstract class ParseOrUnparseState protected (
res
}
+ private var _hiddenDepth = 0
+
+ def incrementHiddenDef = {
+_hiddenDepth += 1
+ }
+ def decrementHiddenDef = {
+_hiddenDepth -= 1
+ }
+
+ def withinHiddenNest: Boolean = _hiddenDepth > 0
Review comment:
Fair enough. Should we modify the description of "info hidden" to mention
"...within the nesting context of a hidden group" or something like that to be
consistent, or is that distinction to verbose/excessive for the debugger?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r390371278 ## File path: daffodil-test/src/test/resources/org/apache/daffodil/section14/sequence_groups/SequencesWithHiddenRefs.dfdl.xsd ## @@ -0,0 +1,94 @@ + + + +http://www.w3.org/2001/XMLSchema; + xmlns:fn="http://www.w3.org/2005/xpath-functions; + xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/; xmlns:ex="http://example.com; + targetNamespace="http://example.com; elementFormDefault="unqualified"> + + +http://www.ogf.org/dfdl/;> + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Review comment: It might also be useful to have a test where a hidden group ref contains another hidden group ref just to ensure that nesting works as expected. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r390366275
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/ProcessorStateBases.scala
##
@@ -480,6 +480,17 @@ abstract class ParseOrUnparseState protected (
res
}
+ private var _hiddenDepth = 0
+
+ def incrementHiddenDef = {
+_hiddenDepth += 1
+ }
+ def decrementHiddenDef = {
+_hiddenDepth -= 1
+ }
+
+ def withinHiddenNest: Boolean = _hiddenDepth > 0
Review comment:
Related to what Mike mentioned in the debugger, changing this variable to
``withinHiddenGroup`` is maybe more consistent with the DFDL spec.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r390363464
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##
@@ -1231,21 +1231,6 @@ class InteractiveDebugger(runner:
InteractiveDebuggerRunner, eCompilers: Express
}
}
- object InfoOccursIndex extends DebugCommand with
DebugCommandValidateZeroArgs {
Review comment:
While we're reording things, it might be worth it to reorder the ``info``
subcommands Seq so they're alphabetical, and to make it so it's not on one
ginormous line.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r390360358
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##
@@ -1497,6 +1482,17 @@ class InteractiveDebugger(runner:
InteractiveDebuggerRunner, eCompilers: Express
}
}
+ object InfoHidden extends DebugCommand with DebugCommandValidateZeroArgs
{
+val name = "hidden"
+override lazy val short = "hid"
Review comment:
I think if you don't define ``short`` then it just defaults to the first
letter of the name, which in this case is "h" and is a reasonable short form
for ``hidden``. Nothing else starts with h.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r390349714
##
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/Term.scala
##
@@ -746,7 +741,7 @@ trait Term
*
* What stops this is when the end of an enclosing element has to be next.
*/
- final def possibleNextChildElementsInInfoset: Seq[ElementBase] =
LV('possibleNextChildElementsInInfoset) {
+ def possibleNextChildElementsInInfoset: Seq[ElementBase] =
LV('possibleNextChildElementsInInfoset) {
Review comment:
Any particular reason final was removed? It doesn't look like this patch
changes anything to override this function.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r390348529
##
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/Term.scala
##
@@ -732,9 +728,8 @@ trait Term
final def possibleFirstChildElementsInInfoset: Seq[ElementBase] =
LV('possibleFirstChildElementsInInfoset) {
val pfct = possibleFirstChildTerms
val firstChildren = pfct.flatMap {
- case e: ElementBase if e.isHidden => Nil
case e: ElementBase => Seq(e)
- case s: SequenceTermBase if s.isHidden => Nil
+ case gr: GroupRef if gr.asModelGroup.isHidden => Nil
Review comment:
Just curious how difficult it would be to move isHidden (or
isHiddenGroupRef, agreed that's amore clear name) to GroupRef? Now that you
mention it, that does feel like that's maybe a more natural place for this
property.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389894439
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/ChoiceCombinator.scala
##
@@ -217,46 +217,49 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
}
override lazy val unparser: Unparser = {
-if (!ch.isHidden) {
- val eventRDMap = ch.choiceBranchMap
- val eventUnparserMap = eventRDMap.flatMap {
-case (cbe, rd) =>
- // if we don't find a matching RD for a term that's probably
- // because the term is an empty sequence or empty choice (which do
happen
- // and we even have tests for them). Since those can never be chosen
by
- // means of an element event, they don't appear in the map.
- val altGram = alternatives.find { alt =>
-val crd = alt.context.runtimeData
-val found = crd =:= rd
-found
- }
- altGram.map { ag => (cbe, ag.unparser) }
- }
- val mapValues = eventUnparserMap.map { case (k, v) => v
}.toSeq.filterNot(_.isEmpty)
- if (mapValues.isEmpty)
-new NadaUnparser(null)
- else {
-new ChoiceCombinatorUnparser(ch.modelGroupRuntimeData,
eventUnparserMap, choiceLengthInBits)
- }
-} else {
- // Choices inside a hidden group ref are slightly different because we
- // will never see events for any of the branches. Instead, we will just
- // always pick the branch in which every thing is defaultble or OVC. It
- // is a warning if more than one of those branches exist. It is an SDE if
- // such a branch does not exist, which is detected elsewhere
-
+val optDefaultableBranchUnparser = if (ch.isHidden) {
Review comment:
Does this logic need to be in the runtime unparsers? We can only know if a
choice is hidden if it is directly referenced by a hidden group ref. But if a
choice is a child of a hidden group ref then this can't know if it's hidden or
not.
Do we need to just always determine a defaultable branch and pass that into
the choice combiantor, and then only use that if we determine we are hidden at
runtime?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389898011
##
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/HiddenGroupCombinatorUnparser.scala
##
@@ -0,0 +1,44 @@
+/*
+ * 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.processors.unparsers
+
+import org.apache.daffodil.processors.ModelGroupRuntimeData
+
+/**
+ * The purpose of this combinator is to increment/decrement the depth counter
+ * for hidden groups, and call the group's unparser. As we get deeper into the
+ * hiddenGroupRefs, we'll increment the counter, call the group's unparser,
and as
+ * we unwind from the refs, we'll decrement.
+ */
+class HiddenGroupCombinatorUnparser(
+ ctxt: ModelGroupRuntimeData, bodyUnparser: Unparser)
+ extends CombinatorUnparser(ctxt) {
+
+ override lazy val childProcessors = Vector(bodyUnparser)
+
+ override lazy val runtimeDependencies = Vector()
+
+ def unparse(start: UState): Unit = {
+start.incrementHiddenDef
+
+// unparse
+bodyUnparser.unparse1(start)
+
+start.decrementHiddenDef
+ }
Review comment:
We probably want to wrap this in a a try/finally so that the hiddenef count
always decremented even if the bodyUnparse throws some exception.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389899042
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/HiddenGroupCombinatorParser.scala
##
@@ -0,0 +1,44 @@
+/*
+ * 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.processors.parsers
+
+import org.apache.daffodil.processors.ModelGroupRuntimeData
+
+/**
+ * The purpose of this combinator is to increment/decrement the depth counter
+ * for hidden groups, call the group's parser. As we get deeper into the
+ * hiddenGroupRefs, we'll increment the counter, call the group's parser, and
as
+ * we unwind from the refs, we'll decrement.
+ */
+class HiddenGroupCombinatorParser(
+ ctxt: ModelGroupRuntimeData, bodyParser: Parser)
+ extends CombinatorParser(ctxt) {
+
+ override lazy val childProcessors = Vector(bodyParser)
+
+ override lazy val runtimeDependencies = Vector()
+
+ def parse(start: PState): Unit = {
+start.incrementHiddenDef
+
+// parse
+bodyParser.parse1(start)
+
+start.decrementHiddenDef
Review comment:
Same here, probably need a try/finally.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389896414
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/HiddenGroupCombinator.scala
##
@@ -0,0 +1,37 @@
+/*
+ * 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.grammar.primitives
+
+import org.apache.daffodil.dsom.ModelGroup
+import org.apache.daffodil.grammar.Gram
+import org.apache.daffodil.grammar.Terminal
+import org.apache.daffodil.processors.parsers.Parser
+import org.apache.daffodil.processors.unparsers.Unparser
+import org.apache.daffodil.processors.parsers.HiddenGroupCombinatorParser
+import org.apache.daffodil.processors.unparsers.HiddenGroupCombinatorUnparser
+
+final class HiddenGroupCombinator(
+ctxt: ModelGroup,
+body: Gram
+) extends Terminal(ctxt, !body.isEmpty){
+
+ lazy val parser: Parser = new HiddenGroupCombinatorParser(
ctxt.modelGroupRuntimeData, body.parser)
+
+ override lazy val unparser: Unparser = new HiddenGroupCombinatorUnparser(
ctxt.modelGroupRuntimeData, body.unparser)
+
+}
Review comment:
Check spaces here. There should be two-space indentation and there's some
extra spaces.
Also, the parenthesis closing the HiddenGroupCombinator should be on the
same line as the last arguement. Eg.
```scala
final class HiddenGroupCombinator(
ctxt: ModelGroup,
body: Gram)
extends Terminal(ctxt, !body.isEmpty){
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389892751
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ModelGroupGrammarMixin.scala
##
@@ -68,9 +69,16 @@ trait ModelGroupGrammarMixin
case c: ChoiceTermBase => DelimiterStackCombinatorChoice(c, content)
case s: SequenceTermBase => DelimiterStackCombinatorSequence(s,
content)
}
- } else { groupContentDef }
+ } else {
+groupContent
+ } //TODO: confirm this works; changed from groupContentDef
-finalContent
+ val mgrd = self.modelGroupRuntimeData
+ if (mgrd.isHidden) {
Review comment:
Does isHidden need to be part of the MoelGroupRuntimeData? Can we just use
isHidden frome the ModelGroup DSOM?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389896732
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/ChoiceTermRuntime1Mixin.scala
##
@@ -140,6 +140,7 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
optIgnoreCase,
maybeFillByteEv,
maybeCheckByteAndBitOrderEv,
- maybeCheckBitOrderAndCharset)
+ maybeCheckBitOrderAndCharset,
+ isHidden)
Review comment:
Is this needed? Seems all hidden logic is either done at compile time or
using the new hidden stack thing.
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:
us...@infra.apache.org
With regards,
Apache Git Services
