This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Handle the new UsingTemplateName.
ClosedPublic

Authored by hokein on Apr 6 2022, 5:08 AM.

Details

Summary

Add supports in FindTarget and IncludeCleaner. This would
improve AST-based features on a tempalte which is found via a using
declaration. For example, go-to-def on vect^or<int> v; gives us the
location of using std::vector, which was not previously.

Base on https://reviews.llvm.org/D123127

Diff Detail

Event Timeline

hokein created this revision.Apr 6 2022, 5:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2022, 5:08 AM
hokein requested review of this revision.Apr 6 2022, 5:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2022, 5:08 AM

thanks a lot for doing this! some drive-by comments

clang-tools-extra/clangd/IncludeCleaner.cpp
91

what's the reason for not doing this in base method instead?

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
90

a note for future:
we probably don't want to consider references to constructor and the underlying decl when figuring out missing includes. as it would imply insertion of underlying header, which might be annoying.
as an example, code patterns in the wild look like:
foo.h:

#include <optional>
namespace famous_library_name {
using std::optional;
}

foo.cc

#include "foo.h"
void foo() {
  famous_library_name::optional x(3); // we were getting complaints around saying `foo.h` is unused, i bet we'll get similar complaints if we said you should insert `<optional>`.
}
sammccall added inline comments.Apr 6 2022, 6:30 AM
clang-tools-extra/clangd/FindTarget.cpp
449

I don't think this really belongs in the else-if chain, if the previous catch matches (known class specialization) then we still want to recognize the alias.

I suspect this probably belongs right at the top, outside the chain

clang-tools-extra/clangd/IncludeCleaner.cpp
82

TraverseTemplateSpecializationType calls TraverseTemplateName, isn't this redundant with below?

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
90

this wrapping is weird, and the amount of code on one line is hard to read, switch to raw-string?

90

we probably don't want to consider references

Yeah, this is "do member accesses count as references" which we've identified as an important policy knob to add to IWYU-as-a-library

imply insertion

we'll probably replace this impl with said library before starting to offer insertions.

(and for now, the behavior in this test matches the way we currently treat constructors elsewhere)

hokein marked an inline comment as done.Apr 14 2022, 5:21 AM
hokein added inline comments.
clang-tools-extra/clangd/FindTarget.cpp
449

oh, you're right.

clang-tools-extra/clangd/IncludeCleaner.cpp
82

yeah, the add(..FoundDecl) is redundant, but the key point here we do an early return, which means we will not traverse the underlying template decl if this template decl is found via the using decl.

I think this is a policy problem of whether we count the underlying decl as a reference for include-cleaner -- currently I just keep it consistent with the UsingType.

91

Traversing the underlying decl doesn't seem to follow the existing principle of RAV (e.g. the underlying TemplateDecl of TemplateName is not traversed neighter), the base method merely traverses written qualifiers of templateName if any.

sammccall accepted this revision.Apr 14 2022, 6:42 AM
sammccall added inline comments.
clang-tools-extra/clangd/IncludeCleaner.cpp
82

Makes sense.

But if we're creating a pseudo-VisitTemplateName by overriding its traverse function, can we move the add(getTemplateName() .getAsTemplateDecl()) logic there too?

This revision is now accepted and ready to land.Apr 14 2022, 6:42 AM

(Would be great to land this in some form if you get a chance!)

(Would be great to land this in some form if you get a chance!)

sure, I plan to land it today.

hokein updated this revision to Diff 423892.Apr 20 2022, 6:38 AM

implement a pseudo-VisitTemplateName locally.

sammccall accepted this revision.Apr 20 2022, 6:42 AM
sammccall added inline comments.
clang-tools-extra/clangd/IncludeCleaner.cpp
82–84

maybe instead:
"Use of a template through an alias only uses the alias (see VisitTemplateName), not the underlying template"

This revision was landed with ongoing or failed builds.Apr 20 2022, 6:42 AM
This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.