kbobyrev updated this revision to Diff 391249.
kbobyrev marked 2 inline comments as done.
kbobyrev added a comment.
Address review comments
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,18 @@
"class ^X;",
"X *y;",
},
+ // FIXME(kirillbobyrev): When definition is available, we don't need to
+ // mark forward declarations as used.
+ {
+ "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,17 @@
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)) {
+ if (SM.isInMainFile(RD->getMostRecentDecl()->getBeginLoc())) {
+ if (const auto *Definition = RD->getMostRecentDecl()->getDefinition())
+ Result.insert(Definition->getLocation());
+ return;
+ }
+ }
for (const Decl *Redecl : D->redecls())
Result.insert(Redecl->getLocation());
}
@@ -128,6 +141,7 @@
ReferencedLocations &Result;
llvm::DenseSet<const void *> Visited;
+ const SourceManager &SM;
};
// Given a set of referenced FileIDs, determines all the potentially-referenced
@@ -253,7 +267,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;
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,18 @@
"class ^X;",
"X *y;",
},
+ // FIXME(kirillbobyrev): When definition is available, we don't need to
+ // mark forward declarations as used.
+ {
+ "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,17 @@
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)) {
+ if (SM.isInMainFile(RD->getMostRecentDecl()->getBeginLoc())) {
+ if (const auto *Definition = RD->getMostRecentDecl()->getDefinition())
+ Result.insert(Definition->getLocation());
+ return;
+ }
+ }
for (const Decl *Redecl : D->redecls())
Result.insert(Redecl->getLocation());
}
@@ -128,6 +141,7 @@
ReferencedLocations &Result;
llvm::DenseSet<const void *> Visited;
+ const SourceManager &SM;
};
// Given a set of referenced FileIDs, determines all the potentially-referenced
@@ -253,7 +267,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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
