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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/DumpAST.cpp:233
       return CCO->getConstructor()->getNameAsString();
+    if (const auto *CTE = dyn_cast<CXXThisExpr>(S)) {
+      bool Const = CTE->getType()->getPointeeType().isLocalConstQualified();
----------------
kadircet wrote:
> 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);
> }
> ```
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.


================
Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:79
       {R"cpp(
-template <typename T> int root() {
-  (void)root<unsigned>();
+namespace root {
+template <typename T> int tmpl() {
----------------
kadircet wrote:
> is this change intentional ?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91868