Page MenuHomePhabricator

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

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

Diff Detail

Event Timeline

hokein created this revision.Fri, Jul 31, 7:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jul 31, 7:17 AM
hokein requested review of this revision.Fri, Jul 31, 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
565

nit: you can simplify to (same for delete)

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

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

hokein updated this revision to Diff 282509.Mon, Aug 3, 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
565

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.

566

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

kadircet accepted this revision.Mon, Aug 3, 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
564

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).

565

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.Mon, Aug 3, 2:31 AM
hokein retitled this revision from [clangd] Support new/deleta operator in TargetFinder. to [clangd] Support new/delete operator in TargetFinder..Mon, Aug 3, 5:04 AM
hokein updated this revision to Diff 282590.Mon, Aug 3, 5:09 AM
hokein marked 2 inline comments as done.

address comments.

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