[PATCH] D114864: [clangd] IncludeClenaer: Don't mark forward declarations of a class if it's declared in the main file

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 390999.
kbobyrev marked an inline comment as done.
kbobyrev added a comment.

Resolve review comments, add justification in tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114864/new/

https://reviews.llvm.org/D114864

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,6 +39,16 @@
           "class ^X;",
           "X *y;",
       },
+      {
+          "class ^X {}; class ^X;",
+          "X *y;",
+      },
+      // We already have forward declaration in the main file, the other
+      // non-definition declarations are not needed.
+      {
+          "class ^X {}; class X;",
+          "class X; X *y;",
+      },
       // TypedefType and UsingDecls
       {
           "using ^Integer = int;",
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -33,7 +33,9 @@
 class ReferencedLocationCrawler
     : public RecursiveASTVisitor<ReferencedLocationCrawler> {
 public:
-  ReferencedLocationCrawler(ReferencedLocations &Result) : Result(Result) {}
+  ReferencedLocationCrawler(ReferencedLocations &Result,
+                            const SourceManager &SM)
+      : Result(Result), SM(SM) {}
 
   bool VisitDeclRefExpr(DeclRefExpr *DRE) {
     add(DRE->getDecl());
@@ -120,6 +122,18 @@
   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.
+    if (const auto *RD = llvm::dyn_cast<RecordDecl>(D)) {
+      const auto *MostRecent = RD->getMostRecentDecl();
+      if (SM.isInMainFile(MostRecent->getBeginLoc())) {
+        if (const auto *Definition = RD->getDefinition())
+          Result.insert(Definition->getLocation());
+        return;
+      }
+    }
     for (const Decl *Redecl : D->redecls())
       Result.insert(Redecl->getLocation());
   }
@@ -128,6 +142,7 @@
 
   ReferencedLocations &Result;
   llvm::DenseSet<const void *> Visited;
+  const SourceManager &SM;
 };
 
 // Given a set of referenced FileIDs, determines all the potentially-referenced
@@ -253,7 +268,7 @@
 ReferencedLocations findReferencedLocations(ParsedAST &AST) {
   trace::Span Tracer("IncludeCleaner::findReferencedLocations");
   ReferencedLocations Result;
-  ReferencedLocationCrawler Crawler(Result);
+  ReferencedLocationCrawler Crawler(Result, AST.getSourceManager());
   Crawler.TraverseAST(AST.getASTContext());
   findReferencedMacros(AST, Result);
   return Result;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D114864.390999.patch
Type: text/x-patch
Size: 2750 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211201/3e2bae5c/attachment-0001.bin>