[clangd] IncludeCleaner: Do not require forward declarations of RecordDecls when...

This makes IncludeCleaner more useful in the presense of a large number of
forward declarations. If the definition is already in the Translation Unit and
visible to the Main File, forward declarations have no effect.

The original patch D112707 was split in two: D114864 and this one.

Reviewed By: kadircet

Differential Revision: https://reviews.llvm.org/D114949

diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 76d5d62..0e44c08 100644 (file)
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -122,16 +122,20 @@ private:
   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<RecordDecl>(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
index ddc0a99..1313485 100644 (file)
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -39,10 +39,10 @@ TEST(IncludeCleaner, ReferencedLocations) {
           "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 @@ TEST(IncludeCleaner, ReferencedLocations) {
           "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 @@ TEST(IncludeCleaner, ReferencedLocations) {
       },
       // Redecls
       {
-          "class ^X; class ^X{}; class ^X;",
-          "X *y;",
+          "void ^foo(); void ^foo() {} void ^foo();",
+          "void bar() { foo(); }",
       },
       // Constructor
       {