Swift: fix hidden AST getters #13232
Conversation
For consistency with the C/C++ QL library, getters of AST elements within the hidden AST should not themselves skip other hidden AST elements.
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
A couple of things I'd like to discuss.
Definitely want to see a DCA run to be confident there are no unintended consequences here.
| exists(AvailabilitySpec immediate | | ||
| immediate = this.getImmediateSpec(index) and | ||
| if exists(this.getResolveStep()) then result = immediate else result = immediate.resolve() | ||
| ) |
There was a problem hiding this comment.
This additional and not-quite-trivial QL is repeated many, many times in the diff. I suppose at code generation time we don't know whether this.getResolveStep() will exist or not, so we can't just generate one or the other case. Still, I wonder if there's a way we can simplify / take out any common code here?
There was a problem hiding this comment.
I think one way to limit that is the other thing I propose doing, that is limit this kind of machinery to when the return type of the getter is an Expr or a Pattern. But not something to be tackled in this PR I think.
There was a problem hiding this comment.
👍
As long as the DCA run looks good performance-wise, I'm OK that we wait for that.
d10c
left a comment
d10c
left a comment
There was a problem hiding this comment.
LGTM! I'm in favour of merging this before the release, and then changing the modifiers separately. Can we still do a DCA run first?
|
DCA looks good 👍 |
For consistency with the C/C++ QL library, getters of AST elements within the hidden AST should not themselves skip other hidden AST elements.