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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
