C++: Support concept id expressions #18366
Conversation
jketema
added
the
depends on internal PR
jketema
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jketema
force-pushed
the
jketema/template-parameters-4
branch
4 times, most recently
from
c843bfa to
0f5b70a
Compare
jketema
force-pushed
the
jketema/template-parameters-5
branch
from
0a8643e to
a4e7505
Compare
jketema
force-pushed
the
jketema/template-parameters-5
branch
from
a4e7505 to
94dabbc
Compare
|
|
||
| private Type getTemplateArgumentType(int index) { | ||
| exists(int i | if this.isTypeConstraint() then i = index - 1 else i = index | | ||
| concept_template_argument(underlyingElement(this), i, unresolveElement(result)) |
There was a problem hiding this comment.
Note that there's a trade-off in the design here: I follow what we do for the arguments in template instantiations, as concept ids are similar to those. Alternatively we could have added them as a kind of child expressions. The latter would have made the modifications to PrintAST somewhat simpler, but would have required custom code on the extractor-side, while in the current situation that shares code with template instantiations.
If a type would be used in multiple places in the AST, rendering of the AST would be broken. Hence, we cannot directly use types as AST nodes.
jketema
force-pushed
the
jketema/template-parameters-5
branch
from
e32fc08 to
f08d100
Compare
| */ | ||
| class ConceptIdExpr extends RequirementExpr, @concept_id { | ||
| override string toString() { result = "concept<...>" } | ||
| override string toString() { | ||
| exists(string name | |
There was a problem hiding this comment.
Is this preferable to result = this.getConcept().getName() + "<...>"
There was a problem hiding this comment.
You're absolutely right, that's much better.
| /** | ||
| * Gets a template argument passed to the concept. | ||
| */ | ||
| final Locatable getATemplateArgument() { result = this.getTemplateArgument(_) } |
There was a problem hiding this comment.
Observation: It feels like we need a new return type class TemplateArgument here, but I see that this code follows the examples elsewhere in the library, so don't fix this now.
| * then `getTemplateArgumentKind(1)` yields `int`, and there is no result for | ||
| * `getTemplateArgumentKind(0)`. | ||
| */ | ||
| final Locatable getTemplateArgumentKind(int index) { |
There was a problem hiding this comment.
I'm a bit confused here. Why is the word Kind used, and what is a "kind"? I note that this predicate always yields a Type, so should this be the return type?
There was a problem hiding this comment.
Nit: Could the name Type getTemplateValueArgumentType() be suitable?
There was a problem hiding this comment.
This is consistent with Declaration::getTemplateArgumentKind/1 and FunctionCall::getTemplateArgumentKind/1.
I could do a follow-up where I change all of them, and deprecate all the getTemplateArgumentKind. What do you think?
There was a problem hiding this comment.
Something like getNonTypeTemplateArgumentType is probably most accurate (as "value" is just an internal name).
|
|
||
| override string getChildAccessorPredicateInternal(int childIndex) { | ||
| exists(this.getChildInternal(childIndex)) and | ||
| result = "getTemplateArgument(" + childIndex.toString() + ")" |
There was a problem hiding this comment.
Removed. Also removed in a few other places.
Pull Request checklist
All query authors
.qhelp. See the documentation in this repository.Internal query authors only
.ql,.qll, or.qhelpfiles. See the documentation (internal access required).