[PATCH] D85028: [clangd] Support new/deleta operator in TargetFinder.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, lgtm!



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:241
   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
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:563
+
+  Flags.push_back("--target=x86_64-pc-linux-gnu");
+  Code = R"cpp(
----------------
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).


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:564
+  Code = R"cpp(
+    struct X {
+      static void *operator new(unsigned long s);
----------------
hokein wrote:
> kadircet wrote:
> > nit: you can simplify to (same for delete)
> > 
> > ```cpp
> > void *operator new(unsigned long);
> > void foo() { new int; }
> > ```
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85028