[PATCH] D84136: [clangd] Fix visitation of ConceptSpecializationExpr in constrained-parameter

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 2 inline comments as done.
nridge added inline comments.


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1843
+  if (const auto *TC = D->getTypeConstraint()) {
+    TRY_TO(TraverseStmt(TC->getImmediatelyDeclaredConstraint()));
     TRY_TO(TraverseConceptReference(*TC));
----------------
hokein wrote:
> Looks like we may visit some nodes in `ConceptReference` twice:
> -  getImmediatelyDeclaredConstraint returns a `ConceptSpecializationExpr` (most cases?) which is a subclass of `ConceptReference`;
> - `TraverseStmt(ConceptSpecializationExpr*)` will dispatch to `TraverseConceptSpecializationExpr` which invokes `TraverseConceptReference` (see Line 2719);
> 
> 
> It is sad that we don't have enough test coverage, could you write some tests in `clang/unittests/Tooling/RecursiveASTVisitorTests/`?
It is true that there will be two calls to `TraverseConceptReference()`. However, they are called on two different `ConceptReference` objects:

  * the call in `TraverseConceptSpecializationExpr` will visit the base subobject of the `ConceptSpecializationExpr` (which inherits from `ConceptReference`)
  * the call in `TraverseTemplateTypeParmDecl` will visit the base subobject of the `TypeConstraint` (which also inherits from `ConceptReference`).

So, I think this is fine -- there are two distinct `ConceptReference` objects in the AST, and with this patch we visit both of them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84136/new/

https://reviews.llvm.org/D84136