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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

as you mentioned in the description, there are two problems, I'd prefer address them in two different patches.



================
Comment at: clang/include/clang/AST/ExprConcepts.h:132
+    // there may not be a template argument list.
+    return ArgsAsWritten->RAngleLoc.isValid() ? ArgsAsWritten->RAngleLoc
+                                              : ConceptName.getEndLoc();
----------------
kadircet wrote:
> i think we should have some tests in clang, at least an ast-dump test in `clang/test/AST/` (for both cases) and possibly also in `clang/unittests/AST/SourceLocationTest.cpp`
+1, ast-dump should be enough to test the invalid end loc, I have a D84461 to slightly improve the dump.


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1843
+  if (const auto *TC = D->getTypeConstraint()) {
+    TRY_TO(TraverseStmt(TC->getImmediatelyDeclaredConstraint()));
     TRY_TO(TraverseConceptReference(*TC));
----------------
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/`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84136