[PATCH] D112707: [clangd] IncludeCleaner: Be more conservative in marking RecordDecl usage

[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.

For expression with RecordDecl type, we only need its definition in case it's
available. And when it isn't, there's a high chance the forward decl is in the
same file, so take the most recent decl.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112707

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
@@ -72,9 +72,13 @@
       },
       // Redecls
       {
-          "class ^X; class ^X{}; class ^X;",
+          "class X; class ^X{}; class X;",
           "X *y;",
       },
+      {
+          "class X;",
+          "class X; X *y;",
+      },
       // Constructor
       {
           "struct ^X { ^X(int) {} int ^foo(); };",
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Tooling/Syntax/Tokens.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 
@@ -42,6 +43,14 @@
   }
 
   bool VisitTagType(TagType *TT) {
+    // For RecordDecls take either definition or the most recent forward
+    // declaration.
+    if (const auto *RD = llvm::dyn_cast<RecordDecl>(TT->getDecl())) {
+      const auto *Definition = RD->getDefinition();
+      Result.insert(Definition ? Definition->getLocation()
+                               : RD->getMostRecentDecl()->getLocation());
+      return true;
+    }
     add(TT->getDecl());
     return true;
   }



Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -72,9 +72,13 @@
       },
       // Redecls
       {
-          "class ^X; class ^X{}; class ^X;",
+          "class X; class ^X{}; class X;",
           "X *y;",
       },
+      {
+          "class X;",
+          "class X; X *y;",
+      },
       // Constructor
       {
           "struct ^X { ^X(int) {} int ^foo(); };",
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Tooling/Syntax/Tokens.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 
@@ -42,6 +43,14 @@
   }
 
   bool VisitTagType(TagType *TT) {
+    // For RecordDecls take either definition or the most recent forward
+    // declaration.
+    if (const auto *RD = llvm::dyn_cast<RecordDecl>(TT->getDecl())) {
+      const auto *Definition = RD->getDefinition();
+      Result.insert(Definition ? Definition->getLocation()
+                               : RD->getMostRecentDecl()->getLocation());
+      return true;
+    }
     add(TT->getDecl());
     return true;
   }

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits