tuxji commented on code in PR #1097:
URL: https://github.com/apache/daffodil/pull/1097#discussion_r1366939459
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaSet.scala:
##########
@@ -571,9 +571,10 @@ final class SchemaSet private (
Seq(encDFV, boDFV, binDFV, outDFV)
}
- lazy val allDefinedVariables = schemas.flatMap {
- _.defineVariables
- }
+ lazy val allDefinedVariables = schemas
+ .flatMap(_.defineVariables)
+ .union(predefinedVars)
Review Comment:
This change to `allDefinedVariables` and the change to `variableMap` in the
next file below moves the lookup of `predefinedVars` from `variableMap` to
`allDefinedVariables`. I do not see any change to `variableMap`'s behavior (it
already retrieved `predefinedVars`) but I do see a change to
`allDefinedVariables`'s behavior (it didn't retrieve `predefinedVars` before).
I grep'ed for calls of `allDefinedVariables` and found the only use in
Daffodil *before this PR* was 3 lines below in `allExternalVariables`'s
definition. This PR introduces 3 new uses of `allDefinedVariables` (shown
below). The `indexOf`'s are no concern and the other place had already looked
up predefined variables as well, so it is no concern that `allDefinedVariables`
includes predefined variables as well as defined variables now.
```rg
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaSet.scala
574: lazy val allDefinedVariables = schemas
578: lazy val allExternalVariables = allDefinedVariables.filter {
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/SchemaSetRuntime1Mixin.scala
38: val vrds = allDefinedVariables.map { _.variableRuntimeData }
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/VariableRuntime1Mixin.scala
49: this.schemaSet.allDefinedVariables.indexOf(this),
101: this.schemaSet.allDefinedVariables.indexOf(defv),
```
I grep'ed for calls of `allExternalVariables` and did not find any before
or after this PR. If there truly are no uses of `allExternalVariables`
anywhere in Daffodil, should we remove `allExternalVariables` from line 578
below?
--
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]
