stevedlawrence commented on a change in pull request #240: Add
dfdlx:choiceBranchKeyRanges
URL: https://github.com/apache/incubator-daffodil/pull/240#discussion_r295783157
##########
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/ChoiceCombinator.scala
##########
@@ -34,9 +34,10 @@ import scala.util.Failure
import scala.util.Success
import scala.math.BigInt
import org.apache.daffodil.cookers.RepValueCooker
-import org.apache.daffodil.processors.RangeBound
Review comment:
Unrelated to this change, but I was looking at the choice parsers and
noticed this line to handle ths keys:
```
val optAns1= dispatchKeyRangeMap.filter({case(min,max,_,_) =>
min.testAsLower(keyAsBigInt) && max.testAsUpper(keyAsBigInt)}).headOption
```
Minor performance thing, but this can just be a find instead of a filter +
headOption. Avoids creating a new list and and stop the search once it finds
the first match instead of filtering the rest of the list.
Another thing I noticed is that right above that we have:
```
val keyAsBigInt = BigInt(key)
```
So we are blindly converting the key to a BigInt. What happens if the
dfdl:choiceDispatchKey evaluates to a string that can't be converted to an int?
Should that be an SDE? If dfdl:choiceBranchKeyRanges exist, does the key need
to be convertable to an int? Or do we just ignore the range mappings knowing
that they can't match? If we allow both choieBranchKey and
choiceBranchKeyRanges, I assume it just means we ignore mappings? Can you add
some negative tests to make sure we do the right thing, whatever is decided?
----------------------------------------------------------------
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
