Page MenuHomePhabricator

[clangd] Report declaration references in findExplicitReferences.
ClosedPublic

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 http://results.llvm-merge-guard.org/Phabricator-36

See http://jenkins.llvm-merge-guard.org/job/Phabricator/36/ for more details.

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

clang-tools-extra/clangd/FindTarget.cpp
415

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

418

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

455

Similar to DeclaratorDecl, let's handle this in VisitNamedDecl

460

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

465

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

469

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

497

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

675

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

762

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

clang-tools-extra/clangd/FindTarget.h
90

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

clang-tools-extra/clangd/unittests/FindTargetTests.cpp
544

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.

610

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
clang-tools-extra/clangd/FindTarget.cpp
415

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

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

762

changed to decl.

clang-tools-extra/clangd/unittests/FindTargetTests.cpp
544

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
clang-tools-extra/clangd/FindTarget.cpp
415
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
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
544

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
clang-tools-extra/clangd/AST.cpp
114

could you move this one back above getQualifierLoc ?

clang-tools-extra/clangd/FindTarget.cpp
415

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
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
865

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

869

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
clang-tools-extra/clangd/FindTarget.cpp
437

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
clang-tools-extra/clangd/FindTarget.cpp
415

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
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
869

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!

clang-tools-extra/clangd/FindTarget.cpp
422

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

438

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

672

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.

673

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

675

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

681

Remove this comment? Seems to be a leftover from experiments

clang-tools-extra/clangd/unittests/XRefsTests.cpp
2287–2288

This looks unrelated. Maybe revert and land separately?

kadircet added inline comments.Oct 17 2019, 7:06 AM
clang-tools-extra/clangd/FindTarget.cpp
415

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.

clang-tools-extra/clangd/unittests/XRefsTests.cpp
2287–2288

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
clang-tools-extra/clangd/FindTarget.cpp
496

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.

clang-tools-extra/clangd/unittests/FindTargetTests.cpp
619

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.

failed: LLVM.tools/llvm-ar/mri-utf8.test
failed: LLVM.tools/llvm-objdump/X86/disassemble-functions.test

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

This revision was automatically updated to reflect the committed changes.