stevedlawrence commented on code in PR #1613:
URL: https://github.com/apache/daffodil/pull/1613#discussion_r2717847564
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SequenceGroup.scala:
##########
@@ -160,6 +162,36 @@ abstract class SequenceGroupTermBase(xml: Node,
lexicalParent: SchemaComponent,
}
}.value
+ lazy val checkValidityOccursCountKind: Unit = {
+ val validChildren: Seq[ElementBase] =
+ groupMembers
+ .filter { m => m.isInstanceOf[LocalElementDecl] ||
m.isInstanceOf[ElementRef] }
+ .map(_.asInstanceOf[ElementBase])
+
+ val invalidChild = validChildren.find(e => {
+ if (hasSeparator) {
Review Comment:
I think we can do this hasSeparator check earlier. If the sequence doesn't
have a separator, then we can skip looking at the children which could save
some time.
##########
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd:
##########
@@ -735,6 +735,7 @@
<xs:enumeration value="deprecatedRelativeSchemaLocation" />
<xs:enumeration value="discouragedDiscriminatorPlacement" />
<xs:enumeration value="discouragedAssertPlacement" />
+ <xs:enumeration value="discouragedOCKParsedWithoutSSPAnyEmpty" />
Review Comment:
I dont' think we should say "discouraged". The discouraged things are legal,
they just don't give the behavior a user usually expects.
This is straight up illegal and we are only temprarily allowing it for
backwards compatability. Maybe instead something like
"separatorSuppressionPolicyError". That's also a bit generic so if there are
any other similar disallowed issues with separatorSuppressionPolciy we can use
the same warn id. We probably want to avoid id's that are too specific.
##########
daffodil-test/src/test/resources/org/apache/daffodil/usertests/SepTests.tdml:
##########
@@ -456,11 +456,11 @@
<xs:element name="root" dfdl:initiator="RECORD">
<xs:complexType>
- <xs:sequence dfdl:separator="/" dfdl:separatorPosition="prefix"
dfdl:terminator="%NL;">
+ <xs:sequence dfdl:separator="/" dfdl:separatorPosition="prefix"
dfdl:terminator="%NL;" dfdl:separatorSuppressionPolicy="anyEmpty">
<xs:element name="field1" type="xs:string" />
<xs:element name="field2" type="xs:string" minOccurs="0" />
<xs:element name="field3" type="xs:string" minOccurs="0" />
- <xs:element name="groupOfFields" minOccurs="1" maxOccurs="3"
dfdl:occursCountKind="parsed">
+ <xs:element name="groupOfFields" minOccurs="1" maxOccurs="3">
Review Comment:
This changes both SSP and OCK. Why change both? Could we jsut change SSP to
anyEmpty (which should work with OKC=parsed) or change OCK=implicit with the
existing SSP? I'm not sure what the test is trying to test.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SequenceGroup.scala:
##########
@@ -160,6 +162,36 @@ abstract class SequenceGroupTermBase(xml: Node,
lexicalParent: SchemaComponent,
}
}.value
+ lazy val checkValidityOccursCountKind: Unit = {
+ val validChildren: Seq[ElementBase] =
+ groupMembers
+ .filter { m => m.isInstanceOf[LocalElementDecl] ||
m.isInstanceOf[ElementRef] }
+ .map(_.asInstanceOf[ElementBase])
+
+ val invalidChild = validChildren.find(e => {
+ if (hasSeparator) {
+ e.occursCountKind match {
+ case Parsed =>
+ separatorSuppressionPolicy match {
+ case SeparatorSuppressionPolicy.AnyEmpty => false
Review Comment:
We can also check SSP earlier. If SSP is AnyEmpty then we don't need to look
at the children at all, also saving some time.
So this might want to become something like:
```scala
if (hasSeparator || separatorSupressionPolicy ne AnyEmpty) {
// look at group members for OCK=parsed elements
}
```
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SequenceGroup.scala:
##########
@@ -160,6 +162,36 @@ abstract class SequenceGroupTermBase(xml: Node,
lexicalParent: SchemaComponent,
}
}.value
+ lazy val checkValidityOccursCountKind: Unit = {
+ val validChildren: Seq[ElementBase] =
+ groupMembers
+ .filter { m => m.isInstanceOf[LocalElementDecl] ||
m.isInstanceOf[ElementRef] }
+ .map(_.asInstanceOf[ElementBase])
+
+ val invalidChild = validChildren.find(e => {
+ if (hasSeparator) {
+ e.occursCountKind match {
+ case Parsed =>
+ separatorSuppressionPolicy match {
+ case SeparatorSuppressionPolicy.AnyEmpty => false
+ case _ => true
+ }
+ case _ => false
+ }
+ } else {
+ false
+ }
+ })
+
+ if (invalidChild.isDefined) {
+ invalidChild.get.SDW(
+ WarnID.DiscouragedOCKParsedWithoutSSPAnyEmpty,
+ "Member of a sequence with dfdl:occursCountKind='parsed' must have
dfdl:separatorSuppressionPolicy='anyEmpty'. " +
+ "This may be changed to an error in a future version of Daffodil."
Review Comment:
This almost makes it sound like the element must have SSP=anyEmpty, but it's
really the containing sequence. Maybe something instead like:
> Member of a sequence with dfdl:occursCountKind='parsed' must have a
containing sequence with dfdl:separatorSuppressionPolicy='anyEmpty', but was "
+ separatorSuppressionPolicy + ". This may be changed to an error ..."
Also, I'm curious what our behavior is if someone does use OCK=parsed with
SSP != anyEmpty. Is the behavior well defined and we essentially assume
SSP=anyEmpty? for the parsed element? Things seem to work, or we do we just not
have any tests that actually care about this combination or properties?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SequenceGroup.scala:
##########
@@ -160,6 +162,36 @@ abstract class SequenceGroupTermBase(xml: Node,
lexicalParent: SchemaComponent,
}
}.value
+ lazy val checkValidityOccursCountKind: Unit = {
+ val validChildren: Seq[ElementBase] =
Review Comment:
Instead of doing a filter and map, I would suggest doing a find, that would
avoid allocating another sequence, e.g.:
```scala
val optInvalidChild = groupMembers.find { m
m match {
case e: ElementBase => e.occursCountKind == parsed
case _ => false
}
}
if (optInvalidChild.defined) {
... // SDE
}
```
--
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]
