Page MenuHomePhabricator

[clangd] Do not duplicate TemplatedDecls in findExplicitReferences
ClosedPublic

Authored by kadircet on Tue, Jan 21, 6:15 AM.

Diff Detail

Event Timeline

kadircet created this revision.Tue, Jan 21, 6:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jan 21, 6:15 AM

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

hokein added inline comments.Tue, Jan 21, 6:45 AM
clang-tools-extra/clangd/FindTarget.cpp
589

the code this is correct right now (since {Class,Function,Var,TypeAlias}TemplateDecls are the decls that derive from RedeclarableTemplateDecl), but I would do this in another way -- just add a filter in the VisitNamedDecl below, like

void VisitNamedDecl(const NamedDecl *ND) {
   ...
   if (ND is one of the  {Class,Function,Var,TypeAlias}TemplateDecls)
     return;
}
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
755

nit: the diff seems not to be the one I have submitted, there should be a FIXME here which should be removed in this patch.

kadircet marked 3 inline comments as done.Tue, Jan 21, 7:01 AM
kadircet added inline comments.
clang-tools-extra/clangd/FindTarget.cpp
589

these two have a slightly different behaviour though, I deliberately chose to stop this in here.

Eliminating here results in dropping template decl, the one containing template parameters.
Whereas eliminating inside VisitNamedDecl, would result in dropping the described template, the real definition coming after template parameters.

It is always possible to switch between the two using get{TemplatedDecl,DescribedTemplate} but I think eliminating the templatedecl aligns better with
current api, since at the specified position we actually have the DescribedTemplate, not the TemplatedDecl and template parameters are already being
reported independent from this.

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

yeah it wasn't rebased

kadircet marked 2 inline comments as done.Tue, Jan 21, 7:11 AM
kadircet added inline comments.
clang-tools-extra/clangd/FindTarget.cpp
589

yeah actually re-thinking about it they will have the same affect since we can check for specific types, nvm this comment.

kadircet updated this revision to Diff 239320.Tue, Jan 21, 7:19 AM
kadircet marked an inline comment as done.
  • Filter out templateddecls in VisitNamedDecls, instead of defining a new Visit method.

Unit tests: pass. 62057 tests passed, 0 failed and 784 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 62057 tests passed, 0 failed and 784 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

hokein accepted this revision.Tue, Jan 21, 8:11 AM
hokein added inline comments.
clang-tools-extra/clangd/FindTarget.cpp
590

It took me a while to understand this comment, I think not everyone is very familiar with TemplatedDecls and DescribedTemplates terms, how about?

Avoid reporting references from {Class,Function,Var,TypeAlias}TemplateDecls to avoid duplicated results, as we will report references from their underlying decls which have the same range.

593

I would check it more elaborately (to reflect the comment above).

if (llvm::isa<ClassTemplateDecl>(ND) || llvm::isa<FunctionTemplateDecl>(ND)..)
  return

or using ND->getKind().

This revision is now accepted and ready to land.Tue, Jan 21, 8:11 AM
kadircet updated this revision to Diff 239523.Wed, Jan 22, 2:44 AM
kadircet marked 2 inline comments as done.
  • Update comments
This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 62094 tests passed, 0 failed and 785 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml