diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -122,16 +122,20 @@ void add(const Decl *D) { if (!D || !isNew(D->getCanonicalDecl())) return; - // If we see a declaration in the mainfile, skip all non-definition decls. - // We only do this for classes because forward declarations are common and - // we don't want to include every header that forward-declares the symbol - // because they're often unrelated. + // Special case RecordDecls, as it is common for them to be forward + // declared multiple times. The most common cases are: + // - Definition available in TU, only mark that one as usage. The rest is + // likely to be unnecessary. This might result in false positives when an + // internal definition is visible. + // - There's a forward declaration in the main file, no need for other + // redecls. if (const auto *RD = llvm::dyn_cast(D)) { - if (SM.isInMainFile(RD->getMostRecentDecl()->getBeginLoc())) { - if (const auto *Definition = RD->getMostRecentDecl()->getDefinition()) - Result.insert(Definition->getLocation()); + if (const auto *Definition = RD->getDefinition()) { + Result.insert(Definition->getLocation()); return; } + if (SM.isInMainFile(RD->getMostRecentDecl()->getLocation())) + return; } for (const Decl *Redecl : D->redecls()) Result.insert(Redecl->getLocation()); diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -39,10 +39,10 @@ "class ^X;", "X *y;", }, - // FIXME(kirillbobyrev): When definition is available, we don't need to - // mark forward declarations as used. + // When definition is available, we don't need to mark forward + // declarations as used. { - "class ^X {}; class ^X;", + "class ^X {}; class X;", "X *y;", }, // We already have forward declaration in the main file, the other @@ -51,17 +51,24 @@ "class ^X {}; class X;", "class X; X *y;", }, + // Nested class definition can occur outside of the parent class + // definition. Bar declaration should be visible to its definition but + // it will always be because we will mark Foo definition as used. + { + "class ^Foo { class Bar; };", + "class Foo::Bar {};", + }, // TypedefType and UsingDecls { "using ^Integer = int;", "Integer x;", }, { - "namespace ns { struct ^X; struct ^X {}; }", - "using ns::X;", + "namespace ns { void ^foo(); void ^foo() {} }", + "using ns::foo;", }, { - "namespace ns { struct X; struct X {}; }", + "namespace ns { void foo(); void foo() {}; }", "using namespace ns;", }, { @@ -88,8 +95,8 @@ }, // Redecls { - "class ^X; class ^X{}; class ^X;", - "X *y;", + "void ^foo(); void ^foo() {} void ^foo();", + "void bar() { foo(); }", }, // Constructor {