Page MenuHomePhabricator

[clangd] Report declaration references in findExplicitReferences.

Authored by hokein on Oct 15 2019, 2:47 AM.

Diff Detail

Event Timeline

hokein created this revision.Oct 15 2019, 2:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2019, 2:47 AM
Bulid results are available at

See for more details.

Mostly LG, but please take a look at the NITs and the implementation-related comments.


This should return SmallVector<ReferenceLoc, 2> now, some declarations can have both decl and non-decl references.


We don't need this flag, in every case we now whether a reference points to a decl or not.


Similar to DeclaratorDecl, let's handle this in VisitNamedDecl


This should be handled in VisitNamedDecl, we should use a helper similar to getQualifier from AST.h to get the qualifier loc instead.


NIT: don't use negative flags, they are hard to read. Prefer IsDeclRef


Why do we need this in the first place?
Expressions never declare anything, just pass false here


Same here: types don't declare anything normally, remove this field and use false directly instead.


NIT: please add /*IsDecl=*/ comment here and in other places to make reading the code simpler.


NIT: maybe use declaration instead?
ref is definitely redundant, we know we're printing a reference.


Initialize to make sure we can't ever have UB when using this struct.


Why do we need this? Can't we test everything we do by putting into foo?

The original idea was to avoid too much output in the tests by letting the setup code live outside the function that's being annotated.
Any reason why we have to do the full file here?

I know it's a weird limitation, but that's one that was already made and I'd keep it this way if it's possible.


We should keep these non-declaration references.
The actual using namespace declaration does not have a name, so we don't report a declaration there

hokein updated this revision to Diff 225398.Oct 17 2019, 4:21 AM
hokein marked 14 inline comments as done.

address comments:

  • remove the confusing IsDeclRef
  • use getQualifier from AST.h
hokein added inline comments.Oct 17 2019, 4:23 AM

Can you give some examples? It seems that non-decl references are handled by other refIn* functions.

int Foo = OtherBar; // OtherBar is captured at the momment.

good point, didn't aware of this utility method, exposed it in AST.h.


changed to decl.


I tried it but failed.

We can't test the qualified definitions (see example below) if we put everything into function foo (as defining a namespace is not allowed inside a function)

namespace $0^ns {
class $1^Foo {
 void $2^method();
void $3^func();
void $4^ns::$5^Foo::$6^method() {}
void $7^ns::$8^func() {}
ilya-biryukov added inline comments.Oct 17 2019, 4:32 AM
namespace $1[[a]] = $2[[std]];

$1 is a declaration of a namespace alias.
$2 is a reference to namespace std.

ilya-biryukov added inline comments.Oct 17 2019, 4:35 AM

Fair point, but this patch does not add support for references in qualifiers of declarations.
I'd keep this change away.

We can totally test all things added by this patch inside the function body.

Could you split the part that adds tests for qualifiers into a separate change?
That would both make the current change more focused and allow to discuss alternatives for the testing story independently of the functionality we're adding here.

kadircet added inline comments.Oct 17 2019, 4:49 AM

could you move this one back above getQualifierLoc ?


I was just about to make the same point. After this patch lands I would like to detect using namespaces inside function body, and currently there is no way to distinguish between a usingdirective and a namespacealias. Since both is only referencing the target namespace.

So even if this starts reporting $2 in the comment above, there should be a difference between that and $3 in using namespace $3[[std]];

ilya-biryukov added inline comments.Oct 17 2019, 4:52 AM

Could we avoid adding this test in this revision?
We could add it in a follow-up patch along with the fix.


Could we avoid reporting destructor references for now? I feel they would create more confusion than bring value.
Mostly worried that they overlap with a reference to the type name and now all client code will probably want to filter out destructor references.

I believe they're not critical to any of the use-cases we have so far, having a `// FIXME: decide how to surface destructors when we need them should be good enough for now

ilya-biryukov added inline comments.Oct 17 2019, 4:55 AM

This one is a not a declaration reference.
We could call VisitNamedDecl here to report the actual declaration.

Note that this would require returning two results from this function.

ilya-biryukov added inline comments.Oct 17 2019, 5:09 AM

That's intentional, though. When we report references, we deliberately leave out information on where it's coming from - otherwise it's hard to encode it.

However, if we really need that, we could also pass a NamedDecl* when reporting declarations (that would mean a slightly different interface, though).

We still have some complexity budget in findExplicitReferences and we should probably spend it here.
OTOH, adding more stuff will probably bloat it too much, I hope we'll stop there... There will still be cases when one would have to write a separate AST traversal to do more complicated stuff. It's unfortunate that this is twice as expensive, but it shouldn't matter in most cases (e.g. when running inside Tweak::apply)

I'd suggest we keep this patch with IsDecl flag, but happy to look into your use-case with using namespaces more closely...

hokein updated this revision to Diff 225420.Oct 17 2019, 6:35 AM
hokein marked 6 inline comments as done.

address comments.

hokein added inline comments.Oct 17 2019, 6:37 AM

Agree. it is not needed in rename feature as well, we have a typeLoc occurrence in ~[[Foo]].

Mostly NITs from my side, the change LG, thanks!


Same comment as below: could you say ReferenceLoc explicitly here and in other places?
Plain initializer lists are somewhat hard to parse


NIT: maybe put this comment on a previous line?
Should read more naturally.


I'd probably suggest to return SmallVector<ReferenceLoc, 2> from all functions, so that they compose better in this situation.

Although Optional<ReferenceLoc> captures the semantics of some of those better, I don't think it's particularly useful at this point.
And having to deconstruct all optionals here leads to a lot of clunky code, it'd be much better if we could keep the structure the same as it's used to be.


NIT: add braces around an inner multi-line if?


Could we have {ReferenceLoc{ instead of {{?
Plain initializer lists are hard to comprehend, especially when they're nested.


Remove this comment? Seems to be a leftover from experiments


This looks unrelated. Maybe revert and land separately?

kadircet added inline comments.Oct 17 2019, 7:06 AM

as discussed offline, it is a special enough use-case to handle in define-inline.

hokein updated this revision to Diff 225428.Oct 17 2019, 7:25 AM
hokein marked 7 inline comments as done.

address remaining nits.


This patch also modifies the getNonLocalDeclRefs which is related to the test here. I'd keep the change in this patch as it is minor and NFC.

ilya-biryukov added inline comments.Oct 18 2019, 2:55 AM

Could we keep an llvm::Optional<> here in the visitor?
The logic in VisitElaboratedTypeLoc looks cheesy if there's more than 1 reference stored...

The function can keep returning SmallVector, that's totally fine.


Similar to using namespace, this should be a non-decl reference.

hokein updated this revision to Diff 225597.Oct 18 2019, 4:37 AM
hokein marked 2 inline comments as done.

address comments.

ilya-biryukov accepted this revision.Oct 18 2019, 4:46 AM

LGTM. Many thanks!

This revision is now accepted and ready to land.Oct 18 2019, 4:46 AM

Build result: fail - 33449 tests passed, 2 failed and 463 were skipped.


Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

This revision was automatically updated to reflect the committed changes.