[PATCH] D123212: [clangd] Handle the new UsingTemplateName.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:446
           Outer.add(RD, Flags); // add(Decl) will despecialize if needed.
+        else if (const auto *UTN =
+                     TST->getTemplateName().getAsUsingTemplateName())
----------------
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


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:76
   bool VisitTemplateSpecializationType(TemplateSpecializationType *TST) {
+    if (const auto *UTN = TST->getTemplateName().getAsUsingTemplateName()) {
+      add(UTN->getFoundDecl());
----------------
TraverseTemplateSpecializationType calls TraverseTemplateName, isn't this redundant with below?


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:89
+       "template <template <typename> class T> class X {}; X<A> x;"},
+      {"namespace ns { template<typename T> struct ^A { ^A(T); }; } using "
+       "ns::^A;",
----------------
kadircet wrote:
> 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>`.
> }
> ```
this wrapping is weird, and the amount of code on one line is hard to read, switch to raw-string?


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:89
+       "template <template <typename> class T> class X {}; X<A> x;"},
+      {"namespace ns { template<typename T> struct ^A { ^A(T); }; } using "
+       "ns::^A;",
----------------
sammccall wrote:
> kadircet wrote:
> > 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>`.
> > }
> > ```
> this wrapping is weird, and the amount of code on one line is hard to read, switch to raw-string?
> 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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123212/new/

https://reviews.llvm.org/D123212