[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 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 will mark more headers that are unrelated to used symbol but contain its
forawrd declaration. E.g. the following are examples of headers forward
declaring `llvm::StringRef`:

- clang/include/clang/Basic/Cuda.h
- llvm/include/llvm/Support/SHA256.h
- llvm/include/llvm/Support/TrigramIndex.h
- llvm/include/llvm/Support/RandomNumberGenerator.
- ... and more (~50 in total)

This patch is a reduced version of D112707 <https://reviews.llvm.org/D112707> which was controversial.


Repository:
  rG LLVM Github Monorepo

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,14 @@
           "class ^X;",
           "X *y;",
       },
+      {
+          "class ^X {}; class ^X;",
+          "X *y;",
+      },
+      {
+          "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());
@@ -48,6 +50,18 @@
   }
 
   bool VisitTagType(TagType *TT) {
+    // 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>(TT->getDecl())) {
+      const auto *MostRecent = RD->getMostRecentDecl();
+      if (SM.isInMainFile(MostRecent->getBeginLoc())) {
+        if (const auto *Definition = RD->getDefinition())
+          Result.insert(Definition->getLocation());
+        return true;
+      }
+    }
     add(TT->getDecl());
     return true;
   }
@@ -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.390977.patch
Type: text/x-patch
Size: 2543 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211201/1ce9a9eb/attachment.bin>