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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.


================
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();
----------------
sammccall wrote:
> 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.
> 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.


================
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() {
----------------
sammccall wrote:
> 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.
ah makes sense. sorry i missed the change from finddecl to unqualifieddecl within the test body.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91868