[GitHub] [incubator-daffodil] olabusayoT commented on a change in pull request #332: Refactor isHidden
olabusayoT commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r398040388
##
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/Term.scala
##
@@ -578,14 +563,13 @@ trait Term
protected def possibleFirstChildTerms: Seq[Term]
/*
- * Returns list of Elements that could be the first child in the infoset
of this model group or element.
- */
+ * Returns list of Elements that could be the first child in the infoset of
this model group or element.
+ */
final lazy val possibleFirstChildElementsInInfoset: Seq[ElementBase] =
LV('possibleFirstChildElementsInInfoset) {
Review comment:
I've created DAFFODIL-2305 to clean up these elements and some of the tests
can't be cleanly deleted, so this can be merged separately.
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] olabusayoT commented on a change in pull request #332: Refactor isHidden
olabusayoT commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r398003298
##
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:
There's a comment under LV, where it says 2 symbols with the same name could
cause issues that tend to be difficult to debug, so I named it something
different since SGR's groupDef already has a groupDef LV. I also found some
precedence where the symbol didn't match the variable name (such as in
org/apache/daffodil/dsom/SchemaDocIncludesAndImportsMixin.scala:151) so I
figured it'd be ok
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] olabusayoT commented on a change in pull request #332: Refactor isHidden
olabusayoT commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r398003835
##
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:
Ah that was an oversight, from when i was trying to debug the dropped HGR.
Will 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] olabusayoT commented on a change in pull request #332: Refactor isHidden
olabusayoT commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r393827299
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/ChoiceCombinator.scala
##
@@ -257,9 +267,8 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
if (mapValues.isEmpty)
new NadaUnparser(null)
else {
- val firstBranchUnparser = alternatives(0).unparser
new ChoiceCombinatorUnparser(ch.modelGroupRuntimeData, eventUnparserMap,
-choiceLengthInBits, optDefaultableBranchUnparser, firstBranchUnparser)
+choiceLengthInBits, optDefaultableBranchUnparser)
Review comment:
Both have been tested and are passing
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] olabusayoT commented on a change in pull request #332: Refactor isHidden
olabusayoT commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r393086662
##
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:
So there is a difference b/w the toString and the toQNameString, the former
considers the namespace in the case of no prefix, and will output '{ns}local'
instead of the latter's 'local', but it doesn't look like there are any other
differences beyond that. I'll update to 'toQNameString" so the same bhv is
maintained across the cases.
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] olabusayoT commented on a change in pull request #332: Refactor isHidden
olabusayoT commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r393015723
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/ChoiceCombinator.scala
##
@@ -217,27 +217,37 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
}
override lazy val unparser: Unparser = {
-val optDefaultableBranchUnparser = if (ch.isHidden) {
- // this call is necessary since it will throw an SDE if no choice branch
- // was defaultable
- ch.childrenThatAreRecursivelyNotDefaultableOrOVC
+/*
+ * 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
- val defaultableBranches = ch.groupMembers.filter {
_.childrenThatAreRecursivelyNotDefaultableOrOVC.length == 0 }
- Assert.invariant(defaultableBranches.length > 0)
- if (ch.isHidden && defaultableBranches.length > 1) {
-SDW(WarnID.ChoiceInsideHiddenGroup, "xs:choice inside a hidden group
has unparse ambiguity: multiple branches exist with all children either
defaulable or have the dfdl:outputValueCalc property set. The first branch will
be chosen during unparse. Defaultable branches are:\n%s",
- defaultableBranches.mkString("\n"))
+// 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.
Review comment:
I meant "require this", it's been update to clarify
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] olabusayoT commented on a change in pull request #332: Refactor isHidden
olabusayoT commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r391088959
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/SequenceTermRuntime1Mixin.scala
##
@@ -76,7 +77,8 @@ trait ChoiceBranchImpliedSequenceRuntime1Mixin { self:
ChoiceBranchImpliedSequen
None,
Maybe.Nope,
Maybe.Nope,
- Maybe.Nope)
+ Maybe.Nope,
+ isHidden)
Review comment:
The Runtime structures no longer have an isHidden member
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] olabusayoT commented on a change in pull request #332: Refactor isHidden
olabusayoT commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r391088444
##
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:
I decided to move isHidden to GroupRef, as isHidden inherited from MG was
only ever possibly true for these cases.
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] olabusayoT commented on a change in pull request #332: Refactor isHidden
olabusayoT commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r391057073
##
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:
After looking a tthe code, I realized we didn't actually need
ModelGroupRuntimeData.isHidden, nor ModelGroup.isHidden (this was moved into
GroupRef)
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] olabusayoT commented on a change in pull request #332: Refactor isHidden
olabusayoT commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r390352687
##
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:
I think this was an oversight. I'd changed it to a val during debugging, and
forgot the "final" during restoration
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
