[clangd] Support new/delete operator in TargetFinder.

[njames93]

clang-tools-extra/clangd/FindTarget.cpp

466–467

nit:

[kadircet]

please note that this might require special handling for go-to-def. as go-to-def only jumps to canonical decl and in operator new's case the user provided one might not be canonical, whereas the canonical one is likely builtin without a source info.

also you have a typo in the review's title s/deleta/delete.

LGTM if you are not planning to make this work with go-to-def now (or ever), please LMK.

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

564

nit: you can simplify to (same for delete)

cpp
void *operator new(unsigned long);
void foo() { new int; }

565

you might want to set --target to a fix triplet to ensure size_t == unsigned long

[hokein]

address comments.

[hokein]

please note that this might require special handling for go-to-def. as go-to-def only jumps to canonical decl and in operator new's case the user provided one might not be canonical, whereas the canonical one is likely builtin without a source info.

this is a good point, I didn't pay much attention to this case. The global new/delete are a bit tricky, they are implicitly declared in every TU.

LGTM if you are not planning to make this work with go-to-def now (or ever), please LMK.

My motivation of this patch is to fix class-specific new/delete operators (go-to-def already works). I added a FIXME in go-to-def. I guess that case happens rarely in practice.

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

564

I think global and class-specific new/delete operators are not the same thing, I would like to keep both of them. Added the global one.

565

good catch, done. The test failed on the windows pre-merge test.

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/XRefs.cpp

241

i think we can make this more generic with something like.

Canonical declarations of some symbols might be provided as a built-in with possibly invalid source locations (e.g. global new operator).
In such cases we should pick a redecl with valid source location, instead of failing.

and possibly put it just above the if(!Loc) statement.

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

563

i think it is better to put it to the top (or move the following tests to a new fixture) so that all of the tests in here are covered by the same Flags (just a precaution for future).

564

i think from the perspective of FindTarget.cpp they are all the same, as it only cares about CXXNewExpr and both of these are of that class.

but sure, extra testing wouldn't hurt.

[hokein]

address comments.

clang-tools-extra/clangd/FindTarget.cpp

struct Visitor : public ConstStmtVisitor<Visitor> {
         Outer.add(OPE->getProtocol(), Flags);
       }
       void VisitOpaqueValueExpr(const OpaqueValueExpr *OVE) {
         Outer.add(OVE->getSourceExpr(), Flags);
       }
       void VisitPseudoObjectExpr(const PseudoObjectExpr *POE) {
         Outer.add(POE->getSyntacticForm(), Flags);
       }
+      void VisitCXXNewExpr(const CXXNewExpr *CNE) {
+        Outer.add(CNE->getOperatorNew(), Flags);
+      }
+      void VisitCXXDeleteExpr(const CXXDeleteExpr *CDE) {
+        Outer.add(CDE->getOperatorDelete(), Flags);
+      }
     };
     Visitor(*this, Flags).Visit(S);
   }
 
   void add(QualType T, RelSet Flags) {
     if (T.isNull())
       return;
     debug(T, Flags);

clang-tools-extra/clangd/XRefs.cpp

locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier,
                   const SymbolIndex *Index, ASTNodeKind *NodeKind) {
   const SourceManager &SM = AST.getSourceManager();
   // Results follow the order of Symbols.Decls.
   std::vector<LocatedSymbol> Result;
   // Keep track of SymbolID -> index mapping, to fill in index data later.
   llvm::DenseMap<SymbolID, size_t> ResultIndex;
 
   auto AddResultDecl = [&](const NamedDecl *D) {
+    // FIXME: C++ global operator new/delete are implicitly declared in every
+    // TU, these functions have invalid source location. In case where users
+    // provide these function definitions (to replace the default ones),
+    // getCanonicalDecl may return the implicit version, which will be dropped
+    // becase of the invalid source location.
     D = llvm::cast<NamedDecl>(D->getCanonicalDecl());
     auto Loc =
         makeLocation(AST.getASTContext(), nameLocation(*D, SM), MainFilePath);
     if (!Loc)
       return;
 
     Result.emplace_back();
     Result.back().Name = printName(AST.getASTContext(), *D);

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

Code = R"cpp(
     };
 
     template <class T>
     void foo(X x, T t) {
       x.[[func]](t);
     };
   )cpp";
   EXPECT_DECLS("UnresolvedMemberExpr", "void func(int *)", "void func(char *)");
+
+  Flags.push_back("--target=x86_64-pc-linux-gnu");
+  Code = R"cpp(
+    struct X {
+      static void *operator new(unsigned long);
+    };
+    auto* k = [[new]] X();
+  )cpp";
+  EXPECT_DECLS("CXXNewExpr", "static void *operator new(unsigned long)");
+  Code = R"cpp(
+    void *operator new(unsigned long);
+    auto* k = [[new]] int();
+  )cpp";
+  EXPECT_DECLS("CXXNewExpr", "void *operator new(unsigned long)");
+
+  Code = R"cpp(
+    struct X {
+      static void operator delete(void *) noexcept;
+    };
+    void k(X* x) {
+      [[delete]] x;
+    }
+  )cpp";
+  EXPECT_DECLS("CXXDeleteExpr", "static void operator delete(void *) noexcept");
+  Code = R"cpp(
+    void operator delete(void *) noexcept;
+    void k(int* x) {
+      [[delete]] x;
+    }
+  )cpp";
+  EXPECT_DECLS("CXXDeleteExpr", "void operator delete(void *) noexcept");
 }
 
 TEST_F(TargetDeclTest, DependentExprs) {
   Flags = {"-fno-delayed-template-parsing"};
 
   // Heuristic resolution of method of dependent field
   Code = R"cpp(
         struct A { void foo() {} };