stevedlawrence commented on a change in pull request #316: Assortment of
changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370618042
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetImpl.scala
##########
@@ -903,7 +903,7 @@ sealed trait DIElement
*/
final def maybeIsNilled: MaybeBoolean = {
if (!_isNilledSet) MaybeBoolean.Nope
- MaybeBoolean(_isNilled)
+ else MaybeBoolean(_isNilled)
Review comment:
After some investigation, while this code is logically wrong, we always use
``maybeIsNilled`` in a manner that doesn't cause any bugs. Previously, this
function could only return two values: ``MaybeBoolean.True`` or
``MaybeBoolean.false``. If nilled hadn't been set yet, it would return
``MaybeBoolean.False`` since false is the default value of _isNilled.
Whenever ``maybeIsNilled`` is used, we always have something to the affect
of:
```
if (maybeIsNilled.isDefined && maybeIsNilled.get == true)
```
The logic is the same regardless if we return MaybeBoolean.Nope or
MaybeBoolean.False. The only place this could cause a bug is we actually cared
if maybeIsNilled hadn't been set yet, or or it had be explicitly set to false.
We don't ever do that, so there isn't currently a bug.
That said, I'm wondering if we should even have maybeIsNilled. The above
conditional seems wrong to me. If maybeIsNilled hasn't been set yet, we
shouldn't be assuming it's false. In fact, I can't imagine a case where want to
know if something is nillled but don't have an answer yet. Seems like this
should be able to go away and we should just always be able to ask isNilled and
get an answer.
----------------------------------------------------------------
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]
With regards,
Apache Git Services
