subject:"\[PATCH\] D112695\: \[clangd\] IncludeCleaner\: Skip non self\-contained headers"

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-29 Thread Kirill Bobyrev via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf47564ea87a5: [clangd] IncludeCleaner: Skip non 
self-contained headers (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.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
@@ -19,6 +19,10 @@
 using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 
+std::string guard(llvm::StringRef Code) {
+  return "#pragma once\n" + Code.str();
+}
+
 TEST(IncludeCleaner, ReferencedLocations) {
   struct TestCase {
 std::string HeaderCode;
@@ -206,6 +210,7 @@
 #include "b.h"
 #include "dir/c.h"
 #include "dir/unused.h"
+#include "unguarded.h"
 #include "unused.h"
 #include 
 void foo() {
@@ -216,13 +221,14 @@
   // Build expected ast with symbols coming from headers.
   TestTU TU;
   TU.Filename = "foo.cpp";
-  TU.AdditionalFiles["foo.h"] = "void foo();";
-  TU.AdditionalFiles["a.h"] = "void a();";
-  TU.AdditionalFiles["b.h"] = "void b();";
-  TU.AdditionalFiles["dir/c.h"] = "void c();";
-  TU.AdditionalFiles["unused.h"] = "void unused();";
-  TU.AdditionalFiles["dir/unused.h"] = "void dirUnused();";
-  TU.AdditionalFiles["system/system_header.h"] = "";
+  TU.AdditionalFiles["foo.h"] = guard("void foo();");
+  TU.AdditionalFiles["a.h"] = guard("void a();");
+  TU.AdditionalFiles["b.h"] = guard("void b();");
+  TU.AdditionalFiles["dir/c.h"] = guard("void c();");
+  TU.AdditionalFiles["unused.h"] = guard("void unused();");
+  TU.AdditionalFiles["dir/unused.h"] = guard("void dirUnused();");
+  TU.AdditionalFiles["system/system_header.h"] = guard("");
+  TU.AdditionalFiles["unguarded.h"] = "";
   TU.ExtraArgs.push_back("-I" + testPath("dir"));
   TU.ExtraArgs.push_back("-isystem" + testPath("system"));
   TU.Code = MainFile.str();
@@ -231,8 +237,7 @@
   for (const auto &Include : computeUnusedIncludes(AST))
 UnusedIncludes.push_back(Include->Written);
   EXPECT_THAT(UnusedIncludes,
-  UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\"",
-   ""));
+  UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\""));
 }
 
 TEST(IncludeCleaner, VirtualBuffers) {
@@ -252,6 +257,9 @@
   // ScratchBuffer with `flags::FLAGS_FOO` that will have FileID but not
   // FileEntry.
   TU.AdditionalFiles["macros.h"] = R"cpp(
+#ifndef MACROS_H
+#define MACROS_H
+
 #define DEFINE_FLAG(X) \
 namespace flags { \
 int FLAGS_##X; \
@@ -261,6 +269,8 @@
 
 #define ab x
 #define concat(x, y) x##y
+
+#endif // MACROS_H
 )cpp";
   TU.ExtraArgs = {"-DCLI=42"};
   ParsedAST AST = TU.build();
@@ -286,7 +296,7 @@
   EXPECT_THAT(ReferencedHeaderNames, ElementsAre(testPath("macros.h")));
 
   // Sanity check.
-  EXPECT_THAT(getUnused(Includes, ReferencedHeaders), ::testing::IsEmpty());
+  EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1483,7 +1483,8 @@
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
 $fix[[  $diag[[#include "unused.h"]]
-]]#include "used.h"
+]]
+  #include "used.h"
 
   #include 
 
@@ -1494,9 +1495,11 @@
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
+#pragma once
 void unused() {}
   )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
+#pragma once
 void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -61,8 +61,9 @@
  const IncludeStructure &Includes, const SourceManager &SM);
 
 /// Retrieves headers that are referenced from the main file but not used.
+/// In unclear cases, headers are not marked as unused.
 std::vector
-getUnused(const IncludeStructure &Includes,
+getUnused(ParsedAST &AST,
   const llvm::DenseSet &ReferencedFiles);
 
 std::vector computeUnusedIncludes(ParsedAST &AST);
Index: clang-tools-extra/clangd/IncludeCleaner.cpp

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-29 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev updated this revision to Diff 383378.
kbobyrev marked 3 inline comments as done.
kbobyrev added a comment.

Resolve the review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.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
@@ -19,6 +19,10 @@
 using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 
+std::string guard(llvm::StringRef Code) {
+  return "#pragma once\n" + Code.str();
+}
+
 TEST(IncludeCleaner, ReferencedLocations) {
   struct TestCase {
 std::string HeaderCode;
@@ -206,6 +210,7 @@
 #include "b.h"
 #include "dir/c.h"
 #include "dir/unused.h"
+#include "unguarded.h"
 #include "unused.h"
 #include 
 void foo() {
@@ -216,13 +221,14 @@
   // Build expected ast with symbols coming from headers.
   TestTU TU;
   TU.Filename = "foo.cpp";
-  TU.AdditionalFiles["foo.h"] = "void foo();";
-  TU.AdditionalFiles["a.h"] = "void a();";
-  TU.AdditionalFiles["b.h"] = "void b();";
-  TU.AdditionalFiles["dir/c.h"] = "void c();";
-  TU.AdditionalFiles["unused.h"] = "void unused();";
-  TU.AdditionalFiles["dir/unused.h"] = "void dirUnused();";
-  TU.AdditionalFiles["system/system_header.h"] = "";
+  TU.AdditionalFiles["foo.h"] = guard("void foo();");
+  TU.AdditionalFiles["a.h"] = guard("void a();");
+  TU.AdditionalFiles["b.h"] = guard("void b();");
+  TU.AdditionalFiles["dir/c.h"] = guard("void c();");
+  TU.AdditionalFiles["unused.h"] = guard("void unused();");
+  TU.AdditionalFiles["dir/unused.h"] = guard("void dirUnused();");
+  TU.AdditionalFiles["system/system_header.h"] = guard("");
+  TU.AdditionalFiles["unguarded.h"] = "";
   TU.ExtraArgs.push_back("-I" + testPath("dir"));
   TU.ExtraArgs.push_back("-isystem" + testPath("system"));
   TU.Code = MainFile.str();
@@ -231,8 +237,7 @@
   for (const auto &Include : computeUnusedIncludes(AST))
 UnusedIncludes.push_back(Include->Written);
   EXPECT_THAT(UnusedIncludes,
-  UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\"",
-   ""));
+  UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\""));
 }
 
 TEST(IncludeCleaner, VirtualBuffers) {
@@ -252,6 +257,9 @@
   // ScratchBuffer with `flags::FLAGS_FOO` that will have FileID but not
   // FileEntry.
   TU.AdditionalFiles["macros.h"] = R"cpp(
+#ifndef MACROS_H
+#define MACROS_H
+
 #define DEFINE_FLAG(X) \
 namespace flags { \
 int FLAGS_##X; \
@@ -261,6 +269,8 @@
 
 #define ab x
 #define concat(x, y) x##y
+
+#endif // MACROS_H
 )cpp";
   TU.ExtraArgs = {"-DCLI=42"};
   ParsedAST AST = TU.build();
@@ -286,7 +296,7 @@
   EXPECT_THAT(ReferencedHeaderNames, ElementsAre(testPath("macros.h")));
 
   // Sanity check.
-  EXPECT_THAT(getUnused(Includes, ReferencedHeaders), ::testing::IsEmpty());
+  EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1483,7 +1483,8 @@
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
 $fix[[  $diag[[#include "unused.h"]]
-]]#include "used.h"
+]]
+  #include "used.h"
 
   #include 
 
@@ -1494,9 +1495,11 @@
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
+#pragma once
 void unused() {}
   )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
+#pragma once
 void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -61,8 +61,9 @@
  const IncludeStructure &Includes, const SourceManager &SM);
 
 /// Retrieves headers that are referenced from the main file but not used.
+/// In unclear cases, headers are not marked as unused.
 std::vector
-getUnused(const IncludeStructure &Includes,
+getUnused(ParsedAST &AST,
   const llvm::DenseSet &ReferencedFiles);
 
 std::vector computeUnusedIncludes(ParsedAST &AST);
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleane

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-29 Thread Sam McCall via Phabricator via cfe-commits

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:250
 if (!MFI.HeaderID) {
   elog("File {0} not found.", MFI.Written);
   continue;

(sorry, I missed this before)

We should not be elogging this case, we're already emitting a diagnostic and it 
doesn't indicate anything wrong with clangd itself.



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:256
+if (!Used && !mayConsiderUnused(MFI, AST)) {
+  dlog("{0} is not a valid unused header and will be filtered out",
+   MFI.Written);

This seems pretty vague text (particularly: what does valid mean, and is it 
valid but used or invalid but unused). Maybe "{0} was not used, but is not 
eligible to be diagnosed as unused"?



Comment at: clang-tools-extra/clangd/IncludeCleaner.h:64
 /// Retrieves headers that are referenced from the main file but not used.
+/// System headers and inclusions of headers with no header guard are dropped.
 std::vector

I'm slightly worried about these sorts of comments becoming stale, they're 
really just describing what we think "not used" means.

I'd replace it with something more general like "In unclear cases, headers are 
not marked as unused", but up to you


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

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

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-29 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev updated this revision to Diff 383253.
kbobyrev added a comment.

Add a header guard in tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.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
@@ -19,6 +19,10 @@
 using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 
+std::string guard(llvm::StringRef Code) {
+  return "#pragma once\n" + Code.str();
+}
+
 TEST(IncludeCleaner, ReferencedLocations) {
   struct TestCase {
 std::string HeaderCode;
@@ -206,6 +210,7 @@
 #include "b.h"
 #include "dir/c.h"
 #include "dir/unused.h"
+#include "unguarded.h"
 #include "unused.h"
 #include 
 void foo() {
@@ -216,13 +221,14 @@
   // Build expected ast with symbols coming from headers.
   TestTU TU;
   TU.Filename = "foo.cpp";
-  TU.AdditionalFiles["foo.h"] = "void foo();";
-  TU.AdditionalFiles["a.h"] = "void a();";
-  TU.AdditionalFiles["b.h"] = "void b();";
-  TU.AdditionalFiles["dir/c.h"] = "void c();";
-  TU.AdditionalFiles["unused.h"] = "void unused();";
-  TU.AdditionalFiles["dir/unused.h"] = "void dirUnused();";
-  TU.AdditionalFiles["system/system_header.h"] = "";
+  TU.AdditionalFiles["foo.h"] = guard("void foo();");
+  TU.AdditionalFiles["a.h"] = guard("void a();");
+  TU.AdditionalFiles["b.h"] = guard("void b();");
+  TU.AdditionalFiles["dir/c.h"] = guard("void c();");
+  TU.AdditionalFiles["unused.h"] = guard("void unused();");
+  TU.AdditionalFiles["dir/unused.h"] = guard("void dirUnused();");
+  TU.AdditionalFiles["system/system_header.h"] = guard("");
+  TU.AdditionalFiles["unguarded.h"] = "";
   TU.ExtraArgs.push_back("-I" + testPath("dir"));
   TU.ExtraArgs.push_back("-isystem" + testPath("system"));
   TU.Code = MainFile.str();
@@ -231,8 +237,7 @@
   for (const auto &Include : computeUnusedIncludes(AST))
 UnusedIncludes.push_back(Include->Written);
   EXPECT_THAT(UnusedIncludes,
-  UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\"",
-   ""));
+  UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\""));
 }
 
 TEST(IncludeCleaner, VirtualBuffers) {
@@ -252,6 +257,9 @@
   // ScratchBuffer with `flags::FLAGS_FOO` that will have FileID but not
   // FileEntry.
   TU.AdditionalFiles["macros.h"] = R"cpp(
+#ifndef MACROS_H
+#define MACROS_H
+
 #define DEFINE_FLAG(X) \
 namespace flags { \
 int FLAGS_##X; \
@@ -261,6 +269,8 @@
 
 #define ab x
 #define concat(x, y) x##y
+
+#endif // MACROS_H
 )cpp";
   TU.ExtraArgs = {"-DCLI=42"};
   ParsedAST AST = TU.build();
@@ -286,7 +296,7 @@
   EXPECT_THAT(ReferencedHeaderNames, ElementsAre(testPath("macros.h")));
 
   // Sanity check.
-  EXPECT_THAT(getUnused(Includes, ReferencedHeaders), ::testing::IsEmpty());
+  EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1483,7 +1483,8 @@
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
 $fix[[  $diag[[#include "unused.h"]]
-]]#include "used.h"
+]]
+  #include "used.h"
 
   #include 
 
@@ -1494,9 +1495,11 @@
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
+#pragma once
 void unused() {}
   )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
+#pragma once
 void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -61,8 +61,9 @@
  const IncludeStructure &Includes, const SourceManager &SM);
 
 /// Retrieves headers that are referenced from the main file but not used.
+/// System headers and inclusions of headers with no header guard are dropped.
 std::vector
-getUnused(const IncludeStructure &Includes,
+getUnused(ParsedAST &AST,
   const llvm::DenseSet &ReferencedFiles);
 
 std::vector computeUnusedIncludes(ParsedAST &AST);
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -8,12 +8,15

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-29 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev updated this revision to Diff 383250.
kbobyrev marked 3 inline comments as done.
kbobyrev added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.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
@@ -19,6 +19,10 @@
 using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 
+std::string guard(llvm::StringRef Code) {
+  return "#pragma once\n" + Code.str();
+}
+
 TEST(IncludeCleaner, ReferencedLocations) {
   struct TestCase {
 std::string HeaderCode;
@@ -206,6 +210,7 @@
 #include "b.h"
 #include "dir/c.h"
 #include "dir/unused.h"
+#include "unguarded.h"
 #include "unused.h"
 #include 
 void foo() {
@@ -216,13 +221,14 @@
   // Build expected ast with symbols coming from headers.
   TestTU TU;
   TU.Filename = "foo.cpp";
-  TU.AdditionalFiles["foo.h"] = "void foo();";
-  TU.AdditionalFiles["a.h"] = "void a();";
-  TU.AdditionalFiles["b.h"] = "void b();";
-  TU.AdditionalFiles["dir/c.h"] = "void c();";
-  TU.AdditionalFiles["unused.h"] = "void unused();";
-  TU.AdditionalFiles["dir/unused.h"] = "void dirUnused();";
-  TU.AdditionalFiles["system/system_header.h"] = "";
+  TU.AdditionalFiles["foo.h"] = guard("void foo();");
+  TU.AdditionalFiles["a.h"] = guard("void a();");
+  TU.AdditionalFiles["b.h"] = guard("void b();");
+  TU.AdditionalFiles["dir/c.h"] = guard("void c();");
+  TU.AdditionalFiles["unused.h"] = guard("void unused();");
+  TU.AdditionalFiles["dir/unused.h"] = guard("void dirUnused();");
+  TU.AdditionalFiles["system/system_header.h"] = guard("");
+  TU.AdditionalFiles["unguarded.h"] = "";
   TU.ExtraArgs.push_back("-I" + testPath("dir"));
   TU.ExtraArgs.push_back("-isystem" + testPath("system"));
   TU.Code = MainFile.str();
@@ -231,8 +237,7 @@
   for (const auto &Include : computeUnusedIncludes(AST))
 UnusedIncludes.push_back(Include->Written);
   EXPECT_THAT(UnusedIncludes,
-  UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\"",
-   ""));
+  UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\""));
 }
 
 TEST(IncludeCleaner, VirtualBuffers) {
@@ -286,7 +291,7 @@
   EXPECT_THAT(ReferencedHeaderNames, ElementsAre(testPath("macros.h")));
 
   // Sanity check.
-  EXPECT_THAT(getUnused(Includes, ReferencedHeaders), ::testing::IsEmpty());
+  EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1483,7 +1483,8 @@
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
 $fix[[  $diag[[#include "unused.h"]]
-]]#include "used.h"
+]]
+  #include "used.h"
 
   #include 
 
@@ -1494,9 +1495,11 @@
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
+#pragma once
 void unused() {}
   )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
+#pragma once
 void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -61,8 +61,9 @@
  const IncludeStructure &Includes, const SourceManager &SM);
 
 /// Retrieves headers that are referenced from the main file but not used.
+/// System headers and inclusions of headers with no header guard are dropped.
 std::vector
-getUnused(const IncludeStructure &Includes,
+getUnused(ParsedAST &AST,
   const llvm::DenseSet &ReferencedFiles);
 
 std::vector computeUnusedIncludes(ParsedAST &AST);
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -8,12 +8,15 @@
 
 #include "IncludeCleaner.h"
 #include "Config.h"
+#include "ParsedAST.h"
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-28 Thread Sam McCall via Phabricator via cfe-commits

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:247
   for (const Inclusion &MFI : Structure.MainFileIncludes) {
 // FIXME: Skip includes that are not self-contained.
 if (!MFI.HeaderID) {

kbobyrev wrote:
> sammccall wrote:
> > this is fixed now (though I think it would be better addressed ~here)
> That's what I had initially (you can see the previous version of the patch, 
> after which I sent "oh, wrong place" and changed it). I think my idea was 
> that this looks confusing from the API standpoint. Choosing to filter out 
> non-guarded headers looks like a diagnostics decision and it looked awkward 
> to me to do that here: what `getUnused` does is simply merge two data 
> structures. I think it's somewhat similar to the patch where you mentioned 
> "parse, don't validate" and it is a great argument: the `` and 
> `` are indeed not "headers" in a sensible meaning of the term 
> but non self-contained ones are.
> 
> I don't have a very strong opinion on this, so I put the filtering back here 
> but I'm curious if you agree with my thoughs.
Sorry, I was unclear: I meant we should put the call to `mayConsiderUnused` 
here, not that we should split it up and put half of it here. So the "written 
with <" check would also be at this point.

> Choosing to filter out non-guarded headers looks like a diagnostics decision
I don't think so: we **don't know** that a header is unused if we suspect it of 
being an umbrella header or used for side effects. The reason we're choosing 
not to diagnose it doesn't have anything to do with **diagnostics** per se.
You could say "unused" has some more narrow definition, but that doesn't seem 
very useful to me.

> what `getUnused` does is simply merge two data structures
Sure, because the nontrivial parts haven't been implemented yet :-) But its 
role is to make filtering decisions of Inclusions from the AST to support a 
high-level concept of "unused", so I think the filtering fits well here.
If we support a non-strict policy, I'd expect getUnused to implement that 
(maybe with helpers) and that's going to involve some more analysis too.

On the other hand, I think the role of the loop in 
issueUnusedIncludesDiagnostics *is* simply converting: between an unused 
include list to diagnostics.

> I think it's somewhat similar to the patch where you mentioned "parse, don't 
> validate" and it is a great argument: the  and  are 
> indeed not "headers" in a sensible meaning of the term but non self-contained 
> ones are.
I'm not sure the analogy holds: my argument was to do the filtering at the 
place where you're doing the conversion/parsing that might fail. But here we 
could perfectly well convert the unused FileIDs to unused Header IDs to unused 
Inclusions to diagnostics, it would just be logically wrong to do so. We have 
free choice of where to put that logic, and diagnostic conversion seems like 
the wrong place to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

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

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-28 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev updated this revision to Diff 383086.
kbobyrev added a comment.

Remove stale FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.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
@@ -19,6 +19,10 @@
 using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 
+std::string guard(llvm::StringRef Code) {
+  return "#pragma once\n" + Code.str();
+}
+
 TEST(IncludeCleaner, ReferencedLocations) {
   struct TestCase {
 std::string HeaderCode;
@@ -216,13 +220,13 @@
   // Build expected ast with symbols coming from headers.
   TestTU TU;
   TU.Filename = "foo.cpp";
-  TU.AdditionalFiles["foo.h"] = "void foo();";
-  TU.AdditionalFiles["a.h"] = "void a();";
-  TU.AdditionalFiles["b.h"] = "void b();";
-  TU.AdditionalFiles["dir/c.h"] = "void c();";
-  TU.AdditionalFiles["unused.h"] = "void unused();";
-  TU.AdditionalFiles["dir/unused.h"] = "void dirUnused();";
-  TU.AdditionalFiles["system/system_header.h"] = "";
+  TU.AdditionalFiles["foo.h"] = guard("void foo();");
+  TU.AdditionalFiles["a.h"] = guard("void a();");
+  TU.AdditionalFiles["b.h"] = guard("void b();");
+  TU.AdditionalFiles["dir/c.h"] = guard("void c();");
+  TU.AdditionalFiles["unused.h"] = guard("void unused();");
+  TU.AdditionalFiles["dir/unused.h"] = guard("void dirUnused();");
+  TU.AdditionalFiles["system/system_header.h"] = guard("");
   TU.ExtraArgs.push_back("-I" + testPath("dir"));
   TU.ExtraArgs.push_back("-isystem" + testPath("system"));
   TU.Code = MainFile.str();
@@ -286,7 +290,7 @@
   EXPECT_THAT(ReferencedHeaderNames, ElementsAre(testPath("macros.h")));
 
   // Sanity check.
-  EXPECT_THAT(getUnused(Includes, ReferencedHeaders), ::testing::IsEmpty());
+  EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1483,7 +1483,8 @@
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
 $fix[[  $diag[[#include "unused.h"]]
-]]#include "used.h"
+]]
+  #include "used.h"
 
   #include 
 
@@ -1494,9 +1495,11 @@
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
+#pragma once
 void unused() {}
   )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
+#pragma once
 void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -61,8 +61,9 @@
  const IncludeStructure &Includes, const SourceManager &SM);
 
 /// Retrieves headers that are referenced from the main file but not used.
+/// Inclusions of headers with no header guard are dropped.
 std::vector
-getUnused(const IncludeStructure &Includes,
+getUnused(ParsedAST &AST,
   const llvm::DenseSet &ReferencedFiles);
 
 std::vector computeUnusedIncludes(ParsedAST &AST);
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -8,12 +8,15 @@
 
 #include "IncludeCleaner.h"
 #include "Config.h"
+#include "ParsedAST.h"
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
@@ -187,9 +190,10 @@
 }
 
 // FIXME(kirillbobyrev): We currently do not support the umbrella headers.
-// Standard Library headers are typically umbrella headers, and system headers
-// are likely to be the Standard Library headers. Until we have a good support
-// for umbrella headers and Standard Library headers, don't warn about them.
+// Standard Library headers are typically umbrella headers, and system
+// headers are likely to be the Standard Library headers. Until we have a
+// good support for umbrella headers and Standard Library headers, don't warn
+// about them.
 bool mayConsiderUnused(const Inclusion *Inc) {

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-28 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:247
   for (const Inclusion &MFI : Structure.MainFileIncludes) {
 // FIXME: Skip includes that are not self-contained.
 if (!MFI.HeaderID) {

sammccall wrote:
> this is fixed now (though I think it would be better addressed ~here)
That's what I had initially (you can see the previous version of the patch, 
after which I sent "oh, wrong place" and changed it). I think my idea was that 
this looks confusing from the API standpoint. Choosing to filter out 
non-guarded headers looks like a diagnostics decision and it looked awkward to 
me to do that here: what `getUnused` does is simply merge two data structures. 
I think it's somewhat similar to the patch where you mentioned "parse, don't 
validate" and it is a great argument: the `` and `` 
are indeed not "headers" in a sensible meaning of the term but non 
self-contained ones are.

I don't have a very strong opinion on this, so I put the filtering back here 
but I'm curious if you agree with my thoughs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

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

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-28 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev updated this revision to Diff 383085.
kbobyrev marked 4 inline comments as done.
kbobyrev added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.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
@@ -19,6 +19,10 @@
 using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 
+std::string guard(llvm::StringRef Code) {
+  return "#pragma once\n" + Code.str();
+}
+
 TEST(IncludeCleaner, ReferencedLocations) {
   struct TestCase {
 std::string HeaderCode;
@@ -216,13 +220,13 @@
   // Build expected ast with symbols coming from headers.
   TestTU TU;
   TU.Filename = "foo.cpp";
-  TU.AdditionalFiles["foo.h"] = "void foo();";
-  TU.AdditionalFiles["a.h"] = "void a();";
-  TU.AdditionalFiles["b.h"] = "void b();";
-  TU.AdditionalFiles["dir/c.h"] = "void c();";
-  TU.AdditionalFiles["unused.h"] = "void unused();";
-  TU.AdditionalFiles["dir/unused.h"] = "void dirUnused();";
-  TU.AdditionalFiles["system/system_header.h"] = "";
+  TU.AdditionalFiles["foo.h"] = guard("void foo();");
+  TU.AdditionalFiles["a.h"] = guard("void a();");
+  TU.AdditionalFiles["b.h"] = guard("void b();");
+  TU.AdditionalFiles["dir/c.h"] = guard("void c();");
+  TU.AdditionalFiles["unused.h"] = guard("void unused();");
+  TU.AdditionalFiles["dir/unused.h"] = guard("void dirUnused();");
+  TU.AdditionalFiles["system/system_header.h"] = guard("");
   TU.ExtraArgs.push_back("-I" + testPath("dir"));
   TU.ExtraArgs.push_back("-isystem" + testPath("system"));
   TU.Code = MainFile.str();
@@ -286,7 +290,7 @@
   EXPECT_THAT(ReferencedHeaderNames, ElementsAre(testPath("macros.h")));
 
   // Sanity check.
-  EXPECT_THAT(getUnused(Includes, ReferencedHeaders), ::testing::IsEmpty());
+  EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1483,7 +1483,8 @@
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
 $fix[[  $diag[[#include "unused.h"]]
-]]#include "used.h"
+]]
+  #include "used.h"
 
   #include 
 
@@ -1494,9 +1495,11 @@
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
+#pragma once
 void unused() {}
   )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
+#pragma once
 void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -61,8 +61,9 @@
  const IncludeStructure &Includes, const SourceManager &SM);
 
 /// Retrieves headers that are referenced from the main file but not used.
+/// Inclusions of headers with no header guard are dropped.
 std::vector
-getUnused(const IncludeStructure &Includes,
+getUnused(ParsedAST &AST,
   const llvm::DenseSet &ReferencedFiles);
 
 std::vector computeUnusedIncludes(ParsedAST &AST);
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -8,12 +8,15 @@
 
 #include "IncludeCleaner.h"
 #include "Config.h"
+#include "ParsedAST.h"
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
@@ -187,9 +190,10 @@
 }
 
 // FIXME(kirillbobyrev): We currently do not support the umbrella headers.
-// Standard Library headers are typically umbrella headers, and system headers
-// are likely to be the Standard Library headers. Until we have a good support
-// for umbrella headers and Standard Library headers, don't warn about them.
+// Standard Library headers are typically umbrella headers, and system
+// headers are likely to be the Standard Library headers. Until we have a
+// good support for umbrella headers and Standard Library headers, don't warn
+// about them.

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-28 Thread Sam McCall via Phabricator via cfe-commits

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:292
   auto Refs = findReferencedLocations(AST);
   auto ReferencedFiles = translateToHeaderIDs(findReferencedFiles(Refs, SM),
   AST.getIncludeStructure(), SM);

sammccall wrote:
> while here, pull out findReferencedFiles into its own var, and leave a FIXME 
> to adjust the set of referenced files to account for non-self-contained 
> headers whose symbols can be attributed to their include parents
Sorry, this was meant to be an optional suggestion rather than an angry command 
:-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

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

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-28 Thread Sam McCall via Phabricator via cfe-commits

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:191
 
-// FIXME(kirillbobyrev): We currently do not support the umbrella headers.
-// Standard Library headers are typically umbrella headers, and system headers
-// are likely to be the Standard Library headers. Until we have a good support
-// for umbrella headers and Standard Library headers, don't warn about them.
-bool mayConsiderUnused(const Inclusion *Inc) {
-  return Inc->Written.front() != '<';
+bool mayConsiderUnused(const Inclusion *Inc, const IncludeStructure &Includes,
+   FileManager &FM, HeaderSearch &HeaderInfo) {

this is a local helper only, just pass in ParsedAST instead of all the extra 
params?



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:247
   for (const Inclusion &MFI : Structure.MainFileIncludes) {
 // FIXME: Skip includes that are not self-contained.
 if (!MFI.HeaderID) {

this is fixed now (though I think it would be better addressed ~here)



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:292
   auto Refs = findReferencedLocations(AST);
   auto ReferencedFiles = translateToHeaderIDs(findReferencedFiles(Refs, SM),
   AST.getIncludeStructure(), SM);

while here, pull out findReferencedFiles into its own var, and leave a FIXME to 
adjust the set of referenced files to account for non-self-contained headers 
whose symbols can be attributed to their include parents



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:311
   for (const auto *Inc : computeUnusedIncludes(AST)) {
-if (!mayConsiderUnused(Inc))
+if (!mayConsiderUnused(Inc, AST.getIncludeStructure(),
+   AST.getSourceManager().getFileManager(),

Oops, I missed this in previous review.

Why are we first computing which includes are unused (including ineligible) and 
then filtering some out, rather than doing this check in the loop in 
computeUnused?



Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:1483
 
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(

This can be now or later, but we can't/shouldn't jam every feature into one 
test case.
At some point we need to find a way to split this up into different cases, or 
at least not have many features that can only be tested at the diagnostic level.
(If we call mayConsiderUnused inside computeUnused as suggested above, this no 
longer has to be a diagnostic test)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

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

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-28 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev updated this revision to Diff 382970.
kbobyrev added a comment.

Revert docs change that is no longer true.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

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


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1483,7 +1483,9 @@
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
 $fix[[  $diag[[#include "unused.h"]]
-]]#include "used.h"
+]]
+  #include "no_guard.h"
+  #include "used.h"
 
   #include 
 
@@ -1494,9 +1496,16 @@
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
+#pragma once
 void unused() {}
   )cpp";
+  // This header is unused but doesn't have a header guard meaning it is not
+  // self-contained and can have side effects. There will be no warning for it.
+  TU.AdditionalFiles["no_guard.h"] = R"cpp(
+int side_effects = 42;
+  )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
+#pragma once
 void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -14,6 +14,8 @@
 #include "support/Trace.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
@@ -186,12 +188,27 @@
   }
 }
 
-// FIXME(kirillbobyrev): We currently do not support the umbrella headers.
-// Standard Library headers are typically umbrella headers, and system headers
-// are likely to be the Standard Library headers. Until we have a good support
-// for umbrella headers and Standard Library headers, don't warn about them.
-bool mayConsiderUnused(const Inclusion *Inc) {
-  return Inc->Written.front() != '<';
+bool mayConsiderUnused(const Inclusion *Inc, const IncludeStructure &Includes,
+   FileManager &FM, HeaderSearch &HeaderInfo) {
+  // FIXME(kirillbobyrev): We currently do not support the umbrella headers.
+  // Standard Library headers are typically umbrella headers, and system
+  // headers are likely to be the Standard Library headers. Until we have a
+  // good support for umbrella headers and Standard Library headers, don't warn
+  // about them.
+  if (Inc->Written.front() == '<')
+return false;
+  // Headers without include guards have side effects and are not
+  // self-contained, skip them.
+  assert(Inc->HeaderID);
+  auto FE = FM.getFile(Includes.getRealPath(
+  static_cast(*Inc->HeaderID)));
+  assert(FE);
+  if (!HeaderInfo.isFileMultipleIncludeGuarded(*FE)) {
+dlog("{0} doesn't have header guard and will not be considered unused",
+ (*FE)->getName());
+return false;
+  }
+  return true;
 }
 
 } // namespace
@@ -291,7 +308,9 @@
   ->getName()
   .str();
   for (const auto *Inc : computeUnusedIncludes(AST)) {
-if (!mayConsiderUnused(Inc))
+if (!mayConsiderUnused(Inc, AST.getIncludeStructure(),
+   AST.getSourceManager().getFileManager(),
+   AST.getPreprocessor().getHeaderSearchInfo()))
   continue;
 Diag D;
 D.Message =


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1483,7 +1483,9 @@
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
 $fix[[  $diag[[#include "unused.h"]]
-]]#include "used.h"
+]]
+  #include "no_guard.h"
+  #include "used.h"
 
   #include 
 
@@ -1494,9 +1496,16 @@
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
+#pragma once
 void unused() {}
   )cpp";
+  // This header is unused but doesn't have a header guard meaning it is not
+  // self-contained and can have side effects. There will be no warning for it.
+  TU.AdditionalFiles["no_guard.h"] = R"cpp(
+int side_effects = 42;
+  )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
+#pragma once
 void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -14,6 +14,8 @@
 #include "su

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-28 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev updated this revision to Diff 382969.
kbobyrev added a comment.

Remove redundant changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

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

Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1483,7 +1483,9 @@
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
 $fix[[  $diag[[#include "unused.h"]]
-]]#include "used.h"
+]]
+  #include "no_guard.h"
+  #include "used.h"
 
   #include 
 
@@ -1494,9 +1496,16 @@
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
+#pragma once
 void unused() {}
   )cpp";
+  // This header is unused but doesn't have a header guard meaning it is not
+  // self-contained and can have side effects. There will be no warning for it.
+  TU.AdditionalFiles["no_guard.h"] = R"cpp(
+int side_effects = 42;
+  )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
+#pragma once
 void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -55,7 +55,8 @@
const SourceManager &SM);
 
 /// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs).
-/// FileIDs that are not true files ( etc) are dropped.
+/// FileIDs that are not true files ( etc) and headers without
+/// include guards are dropped.
 llvm::DenseSet
 translateToHeaderIDs(const llvm::DenseSet &Files,
  const IncludeStructure &Includes, const SourceManager &SM);
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -14,6 +14,8 @@
 #include "support/Trace.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
@@ -186,12 +188,27 @@
   }
 }
 
-// FIXME(kirillbobyrev): We currently do not support the umbrella headers.
-// Standard Library headers are typically umbrella headers, and system headers
-// are likely to be the Standard Library headers. Until we have a good support
-// for umbrella headers and Standard Library headers, don't warn about them.
-bool mayConsiderUnused(const Inclusion *Inc) {
-  return Inc->Written.front() != '<';
+bool mayConsiderUnused(const Inclusion *Inc, const IncludeStructure &Includes,
+   FileManager &FM, HeaderSearch &HeaderInfo) {
+  // FIXME(kirillbobyrev): We currently do not support the umbrella headers.
+  // Standard Library headers are typically umbrella headers, and system
+  // headers are likely to be the Standard Library headers. Until we have a
+  // good support for umbrella headers and Standard Library headers, don't warn
+  // about them.
+  if (Inc->Written.front() == '<')
+return false;
+  // Headers without include guards have side effects and are not
+  // self-contained, skip them.
+  assert(Inc->HeaderID);
+  auto FE = FM.getFile(Includes.getRealPath(
+  static_cast(*Inc->HeaderID)));
+  assert(FE);
+  if (!HeaderInfo.isFileMultipleIncludeGuarded(*FE)) {
+dlog("{0} doesn't have header guard and will not be considered unused",
+ (*FE)->getName());
+return false;
+  }
+  return true;
 }
 
 } // namespace
@@ -291,7 +308,9 @@
   ->getName()
   .str();
   for (const auto *Inc : computeUnusedIncludes(AST)) {
-if (!mayConsiderUnused(Inc))
+if (!mayConsiderUnused(Inc, AST.getIncludeStructure(),
+   AST.getSourceManager().getFileManager(),
+   AST.getPreprocessor().getHeaderSearchInfo()))
   continue;
 Diag D;
 D.Message =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-28 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev updated this revision to Diff 382968.
kbobyrev added a comment.

Put the check into the right place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.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
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "IncludeCleaner.h"
 #include "TestTU.h"
+#include "llvm/ADT/StringRef.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -252,6 +253,9 @@
   // ScratchBuffer with `flags::FLAGS_FOO` that will have FileID but not
   // FileEntry.
   TU.AdditionalFiles["macros.h"] = R"cpp(
+#ifndef MACROS_H
+#define MACROS_H
+
 #define DEFINE_FLAG(X) \
 namespace flags { \
 int FLAGS_##X; \
@@ -261,6 +265,8 @@
 
 #define ab x
 #define concat(x, y) x##y
+
+#endif // MACROS_H
 )cpp";
   TU.ExtraArgs = {"-DCLI=42"};
   ParsedAST AST = TU.build();
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1483,7 +1483,9 @@
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
 $fix[[  $diag[[#include "unused.h"]]
-]]#include "used.h"
+]]
+  #include "no_guard.h"
+  #include "used.h"
 
   #include 
 
@@ -1494,9 +1496,16 @@
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
+#pragma once
 void unused() {}
   )cpp";
+  // This header is unused but doesn't have a header guard meaning it is not
+  // self-contained and can have side effects. There will be no warning for it.
+  TU.AdditionalFiles["no_guard.h"] = R"cpp(
+int side_effects = 42;
+  )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
+#pragma once
 void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -55,7 +55,8 @@
const SourceManager &SM);
 
 /// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs).
-/// FileIDs that are not true files ( etc) are dropped.
+/// FileIDs that are not true files ( etc) and headers without
+/// include guards are dropped.
 llvm::DenseSet
 translateToHeaderIDs(const llvm::DenseSet &Files,
  const IncludeStructure &Includes, const SourceManager &SM);
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -14,6 +14,8 @@
 #include "support/Trace.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
@@ -186,12 +188,27 @@
   }
 }
 
-// FIXME(kirillbobyrev): We currently do not support the umbrella headers.
-// Standard Library headers are typically umbrella headers, and system headers
-// are likely to be the Standard Library headers. Until we have a good support
-// for umbrella headers and Standard Library headers, don't warn about them.
-bool mayConsiderUnused(const Inclusion *Inc) {
-  return Inc->Written.front() != '<';
+bool mayConsiderUnused(const Inclusion *Inc, const IncludeStructure &Includes,
+   FileManager &FM, HeaderSearch &HeaderInfo) {
+  // FIXME(kirillbobyrev): We currently do not support the umbrella headers.
+  // Standard Library headers are typically umbrella headers, and system
+  // headers are likely to be the Standard Library headers. Until we have a
+  // good support for umbrella headers and Standard Library headers, don't warn
+  // about them.
+  if (Inc->Written.front() == '<')
+return false;
+  // Headers without include guards have side effects and are not
+  // self-contained, skip them.
+  assert(Inc->HeaderID);
+  auto FE = FM.getFile(Includes.getRealPath(
+  static_cast(*Inc->HeaderID)));
+  assert(FE);
+  if (!HeaderInfo.isFileMultipleIncludeGuarded(*FE)) {
+dlog("{0} doesn't have header guard and will not be considered unused",
+ (*FE)->getName());
+return false;
+  }
+  return true;
 }
 
 } // namespac

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-28 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev planned changes to this revision.
kbobyrev added a comment.

Aww, I think this is the wrong place to do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

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

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-28 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev updated this revision to Diff 382955.
kbobyrev added a comment.

Update the docs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.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
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "IncludeCleaner.h"
 #include "TestTU.h"
+#include "llvm/ADT/StringRef.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -16,6 +17,10 @@
 namespace clangd {
 namespace {
 
+std::string guard(llvm::StringRef Code) {
+  return "#pragma once\n" + Code.str();
+}
+
 using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 
@@ -216,13 +221,13 @@
   // Build expected ast with symbols coming from headers.
   TestTU TU;
   TU.Filename = "foo.cpp";
-  TU.AdditionalFiles["foo.h"] = "void foo();";
-  TU.AdditionalFiles["a.h"] = "void a();";
-  TU.AdditionalFiles["b.h"] = "void b();";
-  TU.AdditionalFiles["dir/c.h"] = "void c();";
-  TU.AdditionalFiles["unused.h"] = "void unused();";
-  TU.AdditionalFiles["dir/unused.h"] = "void dirUnused();";
-  TU.AdditionalFiles["system/system_header.h"] = "";
+  TU.AdditionalFiles["foo.h"] = guard("void foo();");
+  TU.AdditionalFiles["a.h"] = guard("void a();");
+  TU.AdditionalFiles["b.h"] = guard("void b();");
+  TU.AdditionalFiles["dir/c.h"] = guard("void c();");
+  TU.AdditionalFiles["unused.h"] = guard("void unused();");
+  TU.AdditionalFiles["dir/unused.h"] = guard("void dirUnused();");
+  TU.AdditionalFiles["system/system_header.h"] = guard("");
   TU.ExtraArgs.push_back("-I" + testPath("dir"));
   TU.ExtraArgs.push_back("-isystem" + testPath("system"));
   TU.Code = MainFile.str();
@@ -252,6 +257,9 @@
   // ScratchBuffer with `flags::FLAGS_FOO` that will have FileID but not
   // FileEntry.
   TU.AdditionalFiles["macros.h"] = R"cpp(
+#ifndef MACROS_H
+#define MACROS_H
+
 #define DEFINE_FLAG(X) \
 namespace flags { \
 int FLAGS_##X; \
@@ -261,6 +269,8 @@
 
 #define ab x
 #define concat(x, y) x##y
+
+#endif // MACROS_H
 )cpp";
   TU.ExtraArgs = {"-DCLI=42"};
   ParsedAST AST = TU.build();
@@ -278,7 +288,9 @@
testPath("macros.h")));
 
   // Should not crash due to FileIDs that are not headers.
-  auto ReferencedHeaders = translateToHeaderIDs(ReferencedFiles, Includes, SM);
+  auto ReferencedHeaders =
+  translateToHeaderIDs(ReferencedFiles, Includes, SM,
+   AST.getPreprocessor().getHeaderSearchInfo());
   std::vector ReferencedHeaderNames;
   for (IncludeStructure::HeaderID HID : ReferencedHeaders)
 ReferencedHeaderNames.push_back(Includes.getRealPath(HID));
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1494,9 +1494,11 @@
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
+#pragma once
 void unused() {}
   )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
+#pragma once
 void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -24,6 +24,7 @@
 #include "Headers.h"
 #include "ParsedAST.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/HeaderSearch.h"
 #include "llvm/ADT/DenseSet.h"
 #include 
 
@@ -55,10 +56,12 @@
const SourceManager &SM);
 
 /// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs).
-/// FileIDs that are not true files ( etc) are dropped.
+/// FileIDs that are not true files ( etc) and headers without
+/// include guards are dropped.
 llvm::DenseSet
 translateToHeaderIDs(const llvm::DenseSet &Files,
- const IncludeStructure &Includes, const SourceManager &SM);
+ const IncludeStructure &Includes, const SourceManager &SM,
+ HeaderSearch &HeaderSearch);
 
 /// Retrieves headers that are referenced from the main file but not used.
 std::vector
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -14,6 +14,8 @

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

2021-10-28 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev created this revision.
kbobyrev added a reviewer: sammccall.
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.

Headers without include guards might have side effects or can be the files we
don't want to consider (e.g. tablegen ".inc" files). Skip them when translating
headers to the HeaderIDs that we will consider as unused.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112695

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.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
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "IncludeCleaner.h"
 #include "TestTU.h"
+#include "llvm/ADT/StringRef.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -16,6 +17,10 @@
 namespace clangd {
 namespace {
 
+std::string guard(llvm::StringRef Code) {
+  return "#pragma once\n" + Code.str();
+}
+
 using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 
@@ -216,13 +221,13 @@
   // Build expected ast with symbols coming from headers.
   TestTU TU;
   TU.Filename = "foo.cpp";
-  TU.AdditionalFiles["foo.h"] = "void foo();";
-  TU.AdditionalFiles["a.h"] = "void a();";
-  TU.AdditionalFiles["b.h"] = "void b();";
-  TU.AdditionalFiles["dir/c.h"] = "void c();";
-  TU.AdditionalFiles["unused.h"] = "void unused();";
-  TU.AdditionalFiles["dir/unused.h"] = "void dirUnused();";
-  TU.AdditionalFiles["system/system_header.h"] = "";
+  TU.AdditionalFiles["foo.h"] = guard("void foo();");
+  TU.AdditionalFiles["a.h"] = guard("void a();");
+  TU.AdditionalFiles["b.h"] = guard("void b();");
+  TU.AdditionalFiles["dir/c.h"] = guard("void c();");
+  TU.AdditionalFiles["unused.h"] = guard("void unused();");
+  TU.AdditionalFiles["dir/unused.h"] = guard("void dirUnused();");
+  TU.AdditionalFiles["system/system_header.h"] = guard("");
   TU.ExtraArgs.push_back("-I" + testPath("dir"));
   TU.ExtraArgs.push_back("-isystem" + testPath("system"));
   TU.Code = MainFile.str();
@@ -252,6 +257,9 @@
   // ScratchBuffer with `flags::FLAGS_FOO` that will have FileID but not
   // FileEntry.
   TU.AdditionalFiles["macros.h"] = R"cpp(
+#ifndef MACROS_H
+#define MACROS_H
+
 #define DEFINE_FLAG(X) \
 namespace flags { \
 int FLAGS_##X; \
@@ -261,6 +269,8 @@
 
 #define ab x
 #define concat(x, y) x##y
+
+#endif // MACROS_H
 )cpp";
   TU.ExtraArgs = {"-DCLI=42"};
   ParsedAST AST = TU.build();
@@ -278,7 +288,9 @@
testPath("macros.h")));
 
   // Should not crash due to FileIDs that are not headers.
-  auto ReferencedHeaders = translateToHeaderIDs(ReferencedFiles, Includes, SM);
+  auto ReferencedHeaders =
+  translateToHeaderIDs(ReferencedFiles, Includes, SM,
+   AST.getPreprocessor().getHeaderSearchInfo());
   std::vector ReferencedHeaderNames;
   for (IncludeStructure::HeaderID HID : ReferencedHeaders)
 ReferencedHeaderNames.push_back(Includes.getRealPath(HID));
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1494,9 +1494,11 @@
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
+#pragma once
 void unused() {}
   )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
+#pragma once
 void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -24,6 +24,7 @@
 #include "Headers.h"
 #include "ParsedAST.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/HeaderSearch.h"
 #include "llvm/ADT/DenseSet.h"
 #include 
 
@@ -58,7 +59,8 @@
 /// FileIDs that are not true files ( etc) are dropped.
 llvm::DenseSet
 translateToHeaderIDs(const llvm::DenseSet &Files,
- const IncludeStructure &Includes, const SourceManager &SM);
+ const IncludeStructure &Includes, const SourceManager &SM,
+ HeaderSearch &HeaderSearch);
 
 /// Retrieves headers that are referenced from the main file but not used.
 std::vector
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/Include

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

17 matches

Site Navigation

Mail list logo

Footer information