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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman.
kbobyrev requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

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 <https://reviews.llvm.org/D112707> was split in two: D114864 <https://reviews.llvm.org/D114864> and this one.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114949

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


Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ 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,13 +51,20 @@
           "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 {}; }",
+          "namespace ns { struct X; struct ^X {}; }",
           "using ns::X;",
       },
       {
@@ -88,8 +95,8 @@
       },
       // Redecls
       {
-          "class ^X; class ^X{}; class ^X;",
-          "X *y;",
+          "void ^foo(); void ^foo() {} void ^foo();",
+          "void bar() { foo(); }",
       },
       // Constructor
       {
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -122,13 +122,33 @@
   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.
+    // Handle RecordDecls separately: classes and structs are often
+    // forward-declared and we don't want to include every header that
+    // forward-declares the symbol because they're often unrelated. Also, if
+    // definition is available, we can safely skip forward declarations because
+    // they have no effect. The only exception is symbols that require their
+    // original declaration because of the scope specifier, e.g. nested
+    // classes:
+    //
+    // Foo.h:
+    // class Foo { class Bar; }
+    //
+    // Foo.cpp
+    // class Foo::Bar {};
+    //
+    // Here, Foo::Bar definition will require the original declaration inside
+    // Foo, but it will be available because we will still mark Foo definition
+    // as used.
     if (const auto *RD = llvm::dyn_cast<RecordDecl>(D)) {
-      if (SM.isInMainFile(RD->getMostRecentDecl()->getBeginLoc())) {
-        if (const auto *Definition = RD->getMostRecentDecl()->getDefinition())
+      const auto *Definition = RD->getDefinition();
+      if (Definition) {
+        Result.insert(Definition->getLocation());
+        return;
+      }
+      // If we see a declaration in the mainfile, skip all non-definition
+      // decls.
+      if (SM.isInMainFile(RD->getMostRecentDecl()->getLocation())) {
+        if (Definition)
           Result.insert(Definition->getLocation());
         return;
       }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D114949.391290.patch
Type: text/x-patch
Size: 3689 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211202/31657e92/attachment.bin>