stevedlawrence commented on a change in pull request #385:
URL: https://github.com/apache/incubator-daffodil/pull/385#discussion_r436764216
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
##########
@@ -143,7 +143,7 @@ final case class ConstantExpression[+T <: AnyRef](
lazy val sourceType: NodeInfo.Kind = NodeInfo.fromObject(value)
- def isKnownNonEmpty = value != ""
+ def isKnownNonEmpty = value != "" && value != "%ES;" && !(value == "%WSP*;"
&& qn.toQNameString == "dfdl:initiator")
Review comment:
Sorry about the delay.
I guess my concern here is that ``isKnownNonEmpty`` now means something
different. Previously, it essentially meant the initiator was constant and was
equal to the empty string, i.e. ``dfdl:initiator=""``. And it was used to test
exactly this condition. For example:
```scala
lazy val hasInitiator = {
val hasOne = initiatorExpr.isKnownNonEmpty
hasOne
}
```
hasInitiator should only be true if this condition is true (contstant with
empty string).
But With this change, if we have ``dfdl:initiator="%WSP*;``, then
``isKnownNonEmpty`` will now evaluate to false, and so will ``hasInitiator``.
This feels off to me. Perhaps this doesn't matter based on the order things are
called, but it feels fragile. It feels to me like ``isKnownNonEmpty`` should
keep the logic it currently has (perhaps with a rename like
isConstantEmptyString to make it more clear what it's testing), and something
else needs to have the logic to fix this issue, e.g a function call
canMatchEmptyString on the DFADelimiter.
This change also prevents other properties from being able to use this.
Although only delimiters use this at the momement, it seems reasonable that
other properties might want to know if the property is a constant empty string,
but do not care about whether the value can actually consume empty no data or
not. Seems like this is sort of mixing logic of two different things.
Another thought, this restriction related to initiatedContent also applies
when the initiator property is runtime evaluated, for example:
```xml
<xs:element name="bar" dfdl:initiator="{ if (../foo eq 1) 'foo' else
'%WSP*;' }" ... />
```
Because isKnownNonEmpty only applies to constant expressions, the initiated
content check won't apply for this runtime expression.
The right fix is probably to have some logic described above for
DFADelimters to determine if it can possible match the empty string, and then
have the ``InitiatorParseEv`` check that logic. This would make it so the logic
would be tested on both constant and runtime expressions, and would be specific
to initiators.
That's probably a somewhat significant change though...
----------------------------------------------------------------
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]
