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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
