[clang-tools-extra] bab7a30 - [clangd] IncludeCleaner: Do not require forward declarations of RecordDecls when definition is available

[Kirill Bobyrev via cfe-commits]

Author: Kirill Bobyrev
Date: 2021-12-03T09:36:50+01:00
New Revision: bab7a30ab692059e5e9dc867a59b9ead47efd35c

URL: https://github.com/llvm/llvm-project/commit/bab7a30ab692059e5e9dc867a59b9ead47efd35c
DIFF: https://github.com/llvm/llvm-project/commit/bab7a30ab692059e5e9dc867a59b9ead47efd35c.diff

LOG: [clangd] IncludeCleaner: Do not require forward declarations of RecordDecls when definition is available

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

Added: 
    

Modified: 
    clang-tools-extra/clangd/IncludeCleaner.cpp
    clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 76d5d626b9f88..0e44c080625af 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -122,16 +122,20 @@ class ReferencedLocationCrawler
   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 ddc0a99d58356..1313485f481df 100644
--- 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
       {