This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Support new/delete operator in TargetFinder.
ClosedPublic

Authored by hokein on Jul 31 2020, 7:17 AM.

Diff Detail

Event Timeline

hokein created this revision.Jul 31 2020, 7:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2020, 7:17 AM
hokein requested review of this revision.Jul 31 2020, 7:17 AM
njames93 added inline comments.
clang-tools-extra/clangd/FindTarget.cpp
466–467

nit:

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 updated this revision to Diff 282509.Aug 3 2020, 12:02 AM

address comments.

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 accepted this revision.Aug 3 2020, 2:31 AM

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.

This revision is now accepted and ready to land.Aug 3 2020, 2:31 AM
hokein retitled this revision from [clangd] Support new/deleta operator in TargetFinder. to [clangd] Support new/delete operator in TargetFinder..Aug 3 2020, 5:04 AM
hokein updated this revision to Diff 282590.Aug 3 2020, 5:09 AM
hokein marked 2 inline comments as done.

address comments.

This revision was landed with ongoing or failed builds.Aug 3 2020, 5:10 AM
This revision was automatically updated to reflect the committed changes.