[clangd] Mention when CXXThis is implicit in exposed AST.

[sammccall]

Summary

Seeing an implicit this in the AST is pretty confusing I think.
While here, also mention when this is const.

[kadircet]

This looks like an improvement to me as well, thanks!

clang-tools-extra/clangd/DumpAST.cpp

237

should we ensure we always return within the branch (as we do within the rest of the branches to make sure we don't accumulate details by mistake)? e.g:

if(CXXThisExpr) {
  details = {}
  if (const) details += "const";
  if (implicit) details += "implicit";
  return join(",", details);
}

clang-tools-extra/clangd/unittests/DumpASTTests.cpp

79–81

is this change intentional ?

[sammccall]

clang-tools-extra/clangd/DumpAST.cpp

237

Actually I was trying to *avoid* returning "" in several places.
I think of these functions as a collection of special cases where we have something to say - if we don't find anything, we fall off the end.
(getDetail for DeducedType is the other example)

Can change it if you think it's important though.

clang-tools-extra/clangd/unittests/DumpASTTests.cpp

79–81

Yeah, the problem is that I wanted the root in the new test to be a member function, so I needed to switch from findDecl("root") to findUnqualifiedDecl("root"). But that fails if there are two decls with that name, as there were here: the primary template root<T> and the specialization root<unsigned>. Thus the change to wrap the whole thing in a root namespace. Maybe there's a neater way to solve this, I'm not sure it matters much.

[kadircet]

clang-tools-extra/clangd/DumpAST.cpp

237

Actually I was trying to *avoid* returning "" in several places.

I agree that this is bad, but it feels acceptable when the code doesn't read that way directly :P

I think of these functions as a collection of special cases where we have something to say - if we don't find anything, we fall off the end.

I was afraid of entering other branches, before falling off the end (not applicable here, but assume one day we have a children of CXXThisExpr, this code will prefer the children when thisexpr is neither const nor implicit, which might be surprising).

Can change it if you think it's important though.

As mentioned not an immediate threat, but might become surprising in the future and require some digging. So would be nice if you can.

clang-tools-extra/clangd/unittests/DumpASTTests.cpp

79–81

ah makes sense. sorry i missed the change from finddecl to unqualifieddecl within the test body.

clang-tools-extra/clangd/DumpAST.cpp

std::string getDetail(const Stmt *S) {
     if (const auto *CE = dyn_cast<CastExpr>(S))
       return CE->getCastKindName();
     if (const auto *BO = dyn_cast<BinaryOperator>(S))
       return BO->getOpcodeStr().str();
     if (const auto *UO = dyn_cast<UnaryOperator>(S))
       return UnaryOperator::getOpcodeStr(UO->getOpcode()).str();
     if (const auto *CCO = dyn_cast<CXXConstructExpr>(S))
       return CCO->getConstructor()->getNameAsString();
+    if (const auto *CTE = dyn_cast<CXXThisExpr>(S)) {
+      bool Const = CTE->getType()->getPointeeType().isLocalConstQualified();
+      if (CTE->isImplicit())
+        return Const ? "const, implicit" : "implicit";
+      if (Const)
+        return "const";
+      return "";
+    }
     if (isa<IntegerLiteral>(S) || isa<FloatingLiteral>(S) ||
         isa<FixedPointLiteral>(S) || isa<CharacterLiteral>(S) ||
         isa<ImaginaryLiteral>(S) || isa<CXXBoolLiteralExpr>(S))
       return toString([&](raw_ostream &OS) {
         S->printPretty(OS, nullptr, Ctx.getPrintingPolicy());
       });
     if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(S))
       return MTE->isBoundToLvalueReference() ? "lvalue" : "rvalue";

clang-tools-extra/clangd/unittests/DumpASTTests.cpp

expression: ExprWithCleanups
           expression: Member - x
             expression: MaterializeTemporary - rvalue
               expression: CXXTemporaryObject - S
                 type: Elaborated
                   specifier: Namespace - root::
                   type: Record - S
       )"},
       {R"cpp(
-template <typename T> int root() {
-  (void)root<unsigned>();
+namespace root {
+template <typename T> int tmpl() {
+  (void)tmpl<unsigned>();
   return T::value;
 }
+}
       )cpp",
        R"(
-declaration: FunctionTemplate - root
+declaration: Namespace - root
+  declaration: FunctionTemplate - tmpl
     declaration: TemplateTypeParm - T
-  declaration: Function - root
+    declaration: Function - tmpl
       type: FunctionProto
         type: Builtin - int
       statement: Compound
         expression: CStyleCast - ToVoid
           type: Builtin - void
           expression: Call
             expression: ImplicitCast - FunctionToPointerDecay
-            expression: DeclRef - root
+              expression: DeclRef - tmpl
                 template argument: Type
                   type: Builtin - unsigned int
         statement: Return
           expression: DependentScopeDeclRef - value
             specifier: TypeSpec
               type: TemplateTypeParm - T
       )"},
       {R"cpp(
 struct Foo { char operator+(int); };
 char root = Foo() + 42;
       )cpp",
        R"(
 declaration: Var - root
   type: Builtin - char
   expression: ExprWithCleanups
     expression: CXXOperatorCall
       expression: ImplicitCast - FunctionToPointerDecay
         expression: DeclRef - operator+
       expression: MaterializeTemporary - lvalue
         expression: CXXTemporaryObject - Foo
           type: Record - Foo
       expression: IntegerLiteral - 42
       )"},
+      {R"cpp(
+struct Bar {
+  int x;
+  int root() const {
+    return x;
+  }
+};
+      )cpp",
+       R"(
+declaration: CXXMethod - root
+  type: FunctionProto
+    type: Builtin - int
+  statement: Compound
+    statement: Return
+      expression: ImplicitCast - LValueToRValue
+        expression: Member - x
+          expression: CXXThis - const, implicit
+      )"},
   };
   for (const auto &Case : Cases) {
     ParsedAST AST = TestTU::withCode(Case.first).build();
-    auto Node = dumpAST(DynTypedNode::create(findDecl(AST, "root")),
+    auto Node = dumpAST(DynTypedNode::create(findUnqualifiedDecl(AST, "root")),
                         AST.getTokens(), AST.getASTContext());
     EXPECT_EQ(llvm::StringRef(Case.second).trim(),
               llvm::StringRef(llvm::to_string(Node)).trim());
   }
 }
 
 TEST(DumpASTTests, Range) {
   Annotations Case("$var[[$type[[int]] x]];");