mbeckerle commented on a change in pull request #74: Daffodil trailing sep
URL: https://github.com/apache/incubator-daffodil/pull/74#discussion_r204069230
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/Term.scala
##########
@@ -359,6 +360,25 @@ trait Term
def isKnownRequiredElement = false
def hasKnownRequiredSyntax = false
+ def hasPotentiallyTrailingInstances: Boolean = false
+ final def isPotentiallyTrailing = LV('isPotentiallyTrailing) {
+ if (!isRequired) {
Review comment:
For trailing separator stuff, we don't care about the required sequence
child because its separator (whether pre/in/postfix) is required along with it.
We care about the separator corresponding to an optional element, so that we
can decide whether to absorb it without creating the element or not.
But the language in the spec doesn't match this, so I think
isPotentiallyTrailing member should be renamed to
hasPotentiallySuppressedTrailingEmptySeparator, with some scaladoc explaining
that this is closely related to "potentially trailing" in the spec. but is a
little different.
There is starting to be a terminology problem, both in the DFDL spec, and in
Daffodil. The spec uses "required" and "optional" in different senses. We need
to better distinguish those in Daffodil code.
isRequired needs scaladoc, but I think it means "has at least 1 required
instance". I'm not sure isRequired is meaningful on computed (inputValuecalc)
elements.
The term Required in the runtime however, means "at this array index, the
index is less than or equal to minOccurs, and the OCK is implicit, so minOccurs
matters, or if OCK is fixed or expression, then all indexes are Required. ".
Probably that should change to RequiredOccurrence in the runtime.
The term Optional in the runtime means "at this array index, the index is
greater than minOccurs, and OCK is implicit, so minOccurs and maxOccurs matter,
or OCK='parsed' for any index."
Probably that should change to SpeculatedOccurence in the runtime.
There's also a problem with isArray and isOptional. The term Array appears
in much of the runtime meaning "not scalar and fixed or variable number of
occurrences". That's not consistent with isArray which means known to have the
possibility of having 2 or more occurrences. isOptional means minOccurs 0,
maxOccurs 1 and isn't isArray. I think. Have to verify. But anyway Step one to
sort this out is scaladoc on these to point out the differences.
Unfortunately, all 3 of isRequired, isOptional, and isArray are passed
through to the TermRuntimeData structure, forcing the runtime code to figure
out how to use what they mean. There's quite a lot of code in the runtime that
has to determine scalar, vs. has fixed/specified number of occurrences, vs. has
number of occurrences determined by speculation (possibly after minOccurs
required occurrences). It does this not by looking at isRequired, isArray,
isOptional, but by testing properties that are passed through to parsers so are
part of their runtime state.
Some renaming could help. E.g, in the runtime, use better names like Scalar,
Specified, or Speculated perhaps.
I will add a TODO/FIXME comment about the unordered parent thing. There may
be other things that look upward that may also have this future bug once
unordered sequences are introduced.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
