[PATCH] D125468: [clangd] Include Cleaner: ignore headers with IWYU export pragmas

2022-05-16 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 rG40f361ace3e9: [clangd] Include Cleaner: ignore headers with 
IWYU export pragmas (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125468

Files:
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.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
@@ -544,8 +544,7 @@
   EXPECT_TRUE(
   ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, RecursiveInclusion) {
@@ -576,8 +575,34 @@
   findReferencedLocations(AST), AST.getIncludeStructure(),
   AST.getCanonicalIncludes(), AST.getSourceManager());
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+}
+
+TEST(IncludeCleaner, IWYUPragmaExport) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "foo.h"
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#ifndef FOO_H
+#define FOO_H
+
+#include "bar.h" // IWYU pragma: export
+
+#endif
+  )cpp";
+  TU.AdditionalFiles["bar.h"] = guard(R"cpp(
+void bar() {}
+  )cpp");
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles = findReferencedFiles(
+  findReferencedLocations(AST), AST.getIncludeStructure(),
+  AST.getCanonicalIncludes(), AST.getSourceManager());
+  EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
+  // FIXME: This is not correct: foo.h is unused but is not diagnosed as such
+  // because we ignore headers with IWYU export pragmas for now.
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -406,7 +406,7 @@
   #define RECURSIVE_H
 
   #include "recursive.h"
-  
+
   #endif // RECURSIVE_H
 )cpp";
 
@@ -418,6 +418,33 @@
   EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
 }
 
+TEST_F(HeadersTest, HasIWYUPragmas) {
+  FS.Files[MainFile] = R"cpp(
+#include "export.h"
+#include "begin_exports.h"
+#include "none.h"
+)cpp";
+  FS.Files["export.h"] = R"cpp(
+#pragma once
+#include "none.h" // IWYU pragma: export
+)cpp";
+  FS.Files["begin_exports.h"] = R"cpp(
+#pragma once
+// IWYU pragma: begin_exports
+#include "none.h"
+// IWYU pragma: end_exports
+)cpp";
+  FS.Files["none.h"] = R"cpp(
+#pragma once
+// Not a pragma.
+)cpp";
+
+  auto Includes = collectIncludes();
+  EXPECT_TRUE(Includes.hasIWYUExport(getID("export.h", Includes)));
+  EXPECT_TRUE(Includes.hasIWYUExport(getID("begin_exports.h", Includes)));
+  EXPECT_FALSE(Includes.hasIWYUExport(getID("none.h", Includes)));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -268,13 +268,17 @@
   return true;
 return false;
   }
-  // Headers without include guards have side effects and are not
-  // self-contained, skip them.
   assert(Inc.HeaderID);
+  auto HID = static_cast(*Inc.HeaderID);
+  // FIXME: Ignore the headers with IWYU export pragmas for now, remove this
+  // check when we have more precise tracking of exported headers.
+  if (AST.getIncludeStructure().hasIWYUExport(HID))
+return false;
   auto FE = AST.getSourceManager().getFileManager().getFileRef(
-  AST.getIncludeStructure().getRealPath(
-  static_cast(*Inc.HeaderID)));
+  AST.getIncludeStructure().getRealPath(HID));
   assert(FE);
+  // Headers without include guards have side effects and are not
+  // self-contained, skip them.
   if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded(
   >getFileEntry())) {
 dlog("{0} doesn't have header guard and will not be considered unused",
Index: clang-tools-extra/clangd/Headers.h
===
--- clang-tools-extra/clangd/Headers.h
+++

[PATCH] D125468: [clangd] Include Cleaner: ignore headers with IWYU export pragmas

2022-05-16 Thread Kirill Bobyrev via Phabricator via cfe-commits 
kbobyrev updated this revision to Diff 429637.
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/D125468/new/

https://reviews.llvm.org/D125468

Files:
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.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
@@ -544,8 +544,7 @@
   EXPECT_TRUE(
   ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, RecursiveInclusion) {
@@ -576,8 +575,34 @@
   findReferencedLocations(AST), AST.getIncludeStructure(),
   AST.getCanonicalIncludes(), AST.getSourceManager());
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+}
+
+TEST(IncludeCleaner, IWYUPragmaExport) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "foo.h"
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#ifndef FOO_H
+#define FOO_H
+
+#include "bar.h" // IWYU pragma: export
+
+#endif
+  )cpp";
+  TU.AdditionalFiles["bar.h"] = guard(R"cpp(
+void bar() {}
+  )cpp");
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles = findReferencedFiles(
+  findReferencedLocations(AST), AST.getIncludeStructure(),
+  AST.getCanonicalIncludes(), AST.getSourceManager());
+  EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
+  // FIXME: This is not correct: foo.h is unused but is not diagnosed as such
+  // because we ignore headers with IWYU export pragmas for now.
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -406,7 +406,7 @@
   #define RECURSIVE_H
 
   #include "recursive.h"
-  
+
   #endif // RECURSIVE_H
 )cpp";
 
@@ -418,6 +418,33 @@
   EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
 }
 
+TEST_F(HeadersTest, HasIWYUPragmas) {
+  FS.Files[MainFile] = R"cpp(
+#include "export.h"
+#include "begin_exports.h"
+#include "none.h"
+)cpp";
+  FS.Files["export.h"] = R"cpp(
+#pragma once
+#include "none.h" // IWYU pragma: export
+)cpp";
+  FS.Files["begin_exports.h"] = R"cpp(
+#pragma once
+// IWYU pragma: begin_exports
+#include "none.h"
+// IWYU pragma: end_exports
+)cpp";
+  FS.Files["none.h"] = R"cpp(
+#pragma once
+// Not a pragma.
+)cpp";
+
+  auto Includes = collectIncludes();
+  EXPECT_TRUE(Includes.hasIWYUExport(getID("export.h", Includes)));
+  EXPECT_TRUE(Includes.hasIWYUExport(getID("begin_exports.h", Includes)));
+  EXPECT_FALSE(Includes.hasIWYUExport(getID("none.h", Includes)));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -268,13 +268,17 @@
   return true;
 return false;
   }
-  // Headers without include guards have side effects and are not
-  // self-contained, skip them.
   assert(Inc.HeaderID);
+  auto HID = static_cast(*Inc.HeaderID);
+  // FIXME: Ignore the headers with IWYU export pragmas for now, remove this
+  // check when we have more precise tracking of exported headers.
+  if (AST.getIncludeStructure().hasIWYUExport(HID))
+return false;
   auto FE = AST.getSourceManager().getFileManager().getFileRef(
-  AST.getIncludeStructure().getRealPath(
-  static_cast(*Inc.HeaderID)));
+  AST.getIncludeStructure().getRealPath(HID));
   assert(FE);
+  // Headers without include guards have side effects and are not
+  // self-contained, skip them.
   if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded(
   >getFileEntry())) {
 dlog("{0} doesn't have header guard and will not be considered unused",
Index: clang-tools-extra/clangd/Headers.h
===
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -145,6 +145,10 @@
 return !NonSelfContained.contains(ID);
   }
 
+  bool

[PATCH] D125468: [clangd] Include Cleaner: ignore headers with IWYU export pragmas

2022-05-16 Thread Sam McCall via Phabricator via cfe-commits 
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Great! A few more nits




Comment at: clang-tools-extra/clangd/Headers.cpp:151
+  // will know that the next inclusion is behind the IWYU pragma.
+  if (!Text.consume_front(IWYUPragmaExport) &&
+  !Text.consume_front(IWYUPragmaKeep))

you never use Text again, so consume_front is unneccesary and confusing here, 
we can just use starts_with



Comment at: clang-tools-extra/clangd/Headers.cpp:151
+  // will know that the next inclusion is behind the IWYU pragma.
+  if (!Text.consume_front(IWYUPragmaExport) &&
+  !Text.consume_front(IWYUPragmaKeep))

sammccall wrote:
> you never use Text again, so consume_front is unneccesary and confusing here, 
> we can just use starts_with
add a FIXME that begin_export is not handled here (now that the other branch 
supports it)



Comment at: clang-tools-extra/clangd/Headers.h:148
 
+  bool hasIWYUPragmas(HeaderID ID) const {
+return HasIWYUPragmas.contains(ID);

hasIWYUExport?
(if the file contains non-export pragmas we need to return false)



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:274
+  // FIXME: Ignore the headers with IWYU export pragmas for now, remove this
+  // check when we have actual support for export pragmas.
+  if (AST.getIncludeStructure().hasIWYUPragmas(HID))

actual support for export pragmas => more precise tracking of exported headers?

(current wording doesn't really say what's missing)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125468

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

[PATCH] D125468: [clangd] Include Cleaner: ignore headers with IWYU export pragmas

2022-05-16 Thread Kirill Bobyrev via Phabricator via cfe-commits 
kbobyrev updated this revision to Diff 429619.
kbobyrev added a comment.

Also rebase on top of main.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125468

Files:
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.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
@@ -544,8 +544,7 @@
   EXPECT_TRUE(
   ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, RecursiveInclusion) {
@@ -576,8 +575,34 @@
   findReferencedLocations(AST), AST.getIncludeStructure(),
   AST.getCanonicalIncludes(), AST.getSourceManager());
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+}
+
+TEST(IncludeCleaner, IWYUPragmaExport) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "foo.h"
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#ifndef FOO_H
+#define FOO_H
+
+#include "bar.h" // IWYU pragma: export
+
+#endif
+  )cpp";
+  TU.AdditionalFiles["bar.h"] = guard(R"cpp(
+void bar() {}
+  )cpp");
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles = findReferencedFiles(
+  findReferencedLocations(AST), AST.getIncludeStructure(),
+  AST.getCanonicalIncludes(), AST.getSourceManager());
+  EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
+  // FIXME: This is not correct: foo.h is unused but is not diagnosed as such
+  // because we ignore headers with IWYU export pragmas for now.
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -406,7 +406,7 @@
   #define RECURSIVE_H
 
   #include "recursive.h"
-  
+
   #endif // RECURSIVE_H
 )cpp";
 
@@ -418,6 +418,33 @@
   EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
 }
 
+TEST_F(HeadersTest, HasIWYUPragmas) {
+  FS.Files[MainFile] = R"cpp(
+#include "export.h"
+#include "begin_exports.h"
+#include "none.h"
+)cpp";
+  FS.Files["export.h"] = R"cpp(
+#pragma once
+#include "none.h" // IWYU pragma: export
+)cpp";
+  FS.Files["begin_exports.h"] = R"cpp(
+#pragma once
+// IWYU pragma: begin_exports
+#include "none.h"
+// IWYU pragma: end_exports
+)cpp";
+  FS.Files["none.h"] = R"cpp(
+#pragma once
+// Not a pragma.
+)cpp";
+
+  auto Includes = collectIncludes();
+  EXPECT_TRUE(Includes.hasIWYUPragmas(getID("export.h", Includes)));
+  EXPECT_TRUE(Includes.hasIWYUPragmas(getID("begin_exports.h", Includes)));
+  EXPECT_FALSE(Includes.hasIWYUPragmas(getID("none.h", Includes)));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -268,13 +268,17 @@
   return true;
 return false;
   }
-  // Headers without include guards have side effects and are not
-  // self-contained, skip them.
   assert(Inc.HeaderID);
+  auto HID = static_cast(*Inc.HeaderID);
+  // FIXME: Ignore the headers with IWYU export pragmas for now, remove this
+  // check when we have actual support for export pragmas.
+  if (AST.getIncludeStructure().hasIWYUPragmas(HID))
+return false;
   auto FE = AST.getSourceManager().getFileManager().getFileRef(
-  AST.getIncludeStructure().getRealPath(
-  static_cast(*Inc.HeaderID)));
+  AST.getIncludeStructure().getRealPath(HID));
   assert(FE);
+  // Headers without include guards have side effects and are not
+  // self-contained, skip them.
   if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded(
   >getFileEntry())) {
 dlog("{0} doesn't have header guard and will not be considered unused",
Index: clang-tools-extra/clangd/Headers.h
===
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -145,6 +145,10 @@
 return !NonSelfContained.contains(ID);
   }
 
+  bool hasIWYUPragmas(HeaderID ID) const {
+return

[PATCH] D125468: [clangd] Include Cleaner: ignore headers with IWYU export pragmas

2022-05-16 Thread Kirill Bobyrev via Phabricator via cfe-commits 
kbobyrev updated this revision to Diff 429618.
kbobyrev marked 7 inline comments as done.
kbobyrev added a comment.

Address review comments: the structure is a bit different but the bug is now
actually removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125468

Files:
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.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
@@ -544,8 +544,7 @@
   EXPECT_TRUE(
   ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, RecursiveInclusion) {
@@ -576,8 +575,34 @@
   findReferencedLocations(AST), AST.getIncludeStructure(),
   AST.getCanonicalIncludes(), AST.getSourceManager());
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+}
+
+TEST(IncludeCleaner, IWYUPragmaExport) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "foo.h"
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#ifndef FOO_H
+#define FOO_H
+
+#include "bar.h" // IWYU pragma: export
+
+#endif
+  )cpp";
+  TU.AdditionalFiles["bar.h"] = guard(R"cpp(
+void bar() {}
+  )cpp");
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles = findReferencedFiles(
+  findReferencedLocations(AST), AST.getIncludeStructure(),
+  AST.getCanonicalIncludes(), AST.getSourceManager());
+  EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
+  // FIXME: This is not correct: foo.h is unused but is not diagnosed as such
+  // because we ignore headers with IWYU export pragmas for now.
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -406,7 +406,7 @@
   #define RECURSIVE_H
 
   #include "recursive.h"
-  
+
   #endif // RECURSIVE_H
 )cpp";
 
@@ -418,6 +418,33 @@
   EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
 }
 
+TEST_F(HeadersTest, HasIWYUPragmas) {
+  FS.Files[MainFile] = R"cpp(
+#include "export.h"
+#include "begin_exports.h"
+#include "none.h"
+)cpp";
+  FS.Files["export.h"] = R"cpp(
+#pragma once
+#include "none.h" // IWYU pragma: export
+)cpp";
+  FS.Files["begin_exports.h"] = R"cpp(
+#pragma once
+// IWYU pragma: begin_exports
+#include "none.h"
+// IWYU pragma: end_exports
+)cpp";
+  FS.Files["none.h"] = R"cpp(
+#pragma once
+// Not a pragma.
+)cpp";
+
+  auto Includes = collectIncludes();
+  EXPECT_TRUE(Includes.hasIWYUPragmas(getID("export.h", Includes)));
+  EXPECT_TRUE(Includes.hasIWYUPragmas(getID("begin_exports.h", Includes)));
+  EXPECT_FALSE(Includes.hasIWYUPragmas(getID("none.h", Includes)));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -268,13 +268,17 @@
   return true;
 return false;
   }
-  // Headers without include guards have side effects and are not
-  // self-contained, skip them.
   assert(Inc.HeaderID);
+  auto HID = static_cast(*Inc.HeaderID);
+  // FIXME: Ignore the headers with IWYU export pragmas for now, remove this
+  // check when we have actual support for export pragmas.
+  if (AST.getIncludeStructure().hasIWYUPragmas(HID))
+return false;
   auto FE = AST.getSourceManager().getFileManager().getFileRef(
-  AST.getIncludeStructure().getRealPath(
-  static_cast(*Inc.HeaderID)));
+  AST.getIncludeStructure().getRealPath(HID));
   assert(FE);
+  // Headers without include guards have side effects and are not
+  // self-contained, skip them.
   if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded(
   >getFileEntry())) {
 dlog("{0} doesn't have header guard and will not be considered unused",
Index: clang-tools-extra/clangd/Headers.h
===
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -145,6 +145,10

[PATCH] D125468: [clangd] Include Cleaner: ignore headers with IWYU export pragmas

2022-05-13 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/Headers.cpp:134
+  return false;
+return inMainFile() ? handleCommentInMainFile(PP, Range)
+: handleCommentInHeaderFile(PP, Range);

the split seems to be increasing code duplication (we've already copied a bug 
with it ), what about:
```
bool Err = false;
llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), );
if (!Err || (!Text.consume_front(IWYUPragmaKeep) &&
!Text.consume_front(IWYUPragmaExport)))
  return false;
if(inMainFile()) {
  unsigned Offset = SM.getFileOffset(Range.getBegin());
  LastPragmaKeepInMainFileLine =
  SM.getLineNumber(SM.getMainFileID(), Offset) - 1;
} else {
  Out->HasIWYUPragmas.insert(
*Out->getID(SM.getFileEntryForID(SM.getFileID(Range.getBegin();
}
return false;
```



Comment at: clang-tools-extra/clangd/Headers.cpp:154
+  bool handleCommentInMainFile(Preprocessor , SourceRange Range) {
+assert(inMainFile());
+assert(!Range.getBegin().isMacroID());

i don't think the asserts carry their weight (here and also in the headerfile 
comment handler). we're not really performing anything dubious if the asserts 
don't hold. getCharacterData will return non-sense (but valid) buffers in case 
of a macroid, and we're already reading data whether we're in the main file or 
not. moreover these are private and controlled at the entry by the public 
interface (`HandleComment`).



Comment at: clang-tools-extra/clangd/Headers.cpp:172
+llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), );
+if (Err && !Text.consume_front(IWYUPragmaExport) &&
+!Text.consume_front(IWYUPragmaBeginExports))

shouldn't this be `if (Err || (!Text.consume ... && !Text.consume...)) return 
false;` ? same above for the main file comment handling



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:271
   }
   // Headers without include guards have side effects and are not
   // self-contained, skip them.

can you also move this comment below, next to the `isFileMultipleHeaderGuarded` 
check?



Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:427
+)cpp";
+  FS.Files[testPath("export.h")] = R"cpp(
+#pragma once

no need for `testPath`s here and below.



Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:438
+  FS.Files[testPath("none.h")] = R"cpp(
+#pragma once
+)cpp";

this is passing not because you don't have a IWYU pragma comment here, but 
because you don't have any comments at all. can you try inserting an arbitrary 
comment into the file?



Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:603
+  EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
+  // FIXME: This is not correct: foo.h and bar.h are unused but are not
+  // diagnosed as such because we ignore headers with IWYU export pragmas for

do you mean just `foo.h is unused` why are we talking about `bar.h` also being 
unused here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125468

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

[PATCH] D125468: [clangd] Include Cleaner: ignore headers with IWYU export pragmas

2022-05-12 Thread Kirill Bobyrev via Phabricator via cfe-commits 
kbobyrev updated this revision to Diff 428949.
kbobyrev added a comment.

Remove redundant comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125468

Files:
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.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
@@ -544,8 +544,7 @@
   EXPECT_TRUE(
   ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, RecursiveInclusion) {
@@ -576,8 +575,35 @@
   findReferencedLocations(AST), AST.getIncludeStructure(),
   AST.getCanonicalIncludes(), AST.getSourceManager());
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+}
+
+TEST(IncludeCleaner, IWYUPragmaExport) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "foo.h"
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#ifndef FOO_H
+#define FOO_H
+
+#include "bar.h" // IWYU pragma: export
+
+#endif
+  )cpp";
+  TU.AdditionalFiles["bar.h"] = guard(R"cpp(
+void bar() {}
+  )cpp");
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles = findReferencedFiles(
+  findReferencedLocations(AST), AST.getIncludeStructure(),
+  AST.getCanonicalIncludes(), AST.getSourceManager());
+  EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
+  // FIXME: This is not correct: foo.h and bar.h are unused but are not
+  // diagnosed as such because we ignore headers with IWYU export pragmas for
+  // now.
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -406,7 +406,7 @@
   #define RECURSIVE_H
 
   #include "recursive.h"
-  
+
   #endif // RECURSIVE_H
 )cpp";
 
@@ -418,6 +418,32 @@
   EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
 }
 
+TEST_F(HeadersTest, HasIWYUPragmas) {
+  FS.Files[MainFile] = R"cpp(
+#include "export.h"
+#include "begin_exports.h"
+#include "none.h"
+)cpp";
+  FS.Files[testPath("export.h")] = R"cpp(
+#pragma once
+#include "none.h" // IWYU pragma: export
+)cpp";
+  FS.Files[testPath("begin_exports.h")] = R"cpp(
+#pragma once
+// IWYU pragma: begin_exports
+#include "none.h"
+// IWYU pragma: end_exports
+)cpp";
+  FS.Files[testPath("none.h")] = R"cpp(
+#pragma once
+)cpp";
+
+  auto Includes = collectIncludes();
+  EXPECT_TRUE(Includes.hasIWYUPragmas(getID("export.h", Includes)));
+  EXPECT_TRUE(Includes.hasIWYUPragmas(getID("begin_exports.h", Includes)));
+  EXPECT_FALSE(Includes.hasIWYUPragmas(getID("none.h", Includes)));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -271,9 +271,13 @@
   // Headers without include guards have side effects and are not
   // self-contained, skip them.
   assert(Inc.HeaderID);
+  auto HID = static_cast(*Inc.HeaderID);
+  // FIXME: Ignore the headers with IWYU export pragmas for now, remove this
+  // check when we have actual support for export pragmas.
+  if (AST.getIncludeStructure().hasIWYUPragmas(HID))
+return false;
   auto FE = AST.getSourceManager().getFileManager().getFileRef(
-  AST.getIncludeStructure().getRealPath(
-  static_cast(*Inc.HeaderID)));
+  AST.getIncludeStructure().getRealPath(HID));
   assert(FE);
   if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded(
   >getFileEntry())) {
Index: clang-tools-extra/clangd/Headers.h
===
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -145,6 +145,10 @@
 return !NonSelfContained.contains(ID);
   }
 
+  bool hasIWYUPragmas(HeaderID ID) const {
+return HasIWYUPragmas.contains(ID);
+  }
+
   // Return all transitively reachable files.
   llvm::ArrayRef allHeaders() const { return RealPathNames; }
 
@@ -185,6 +189,9 @@
   // Contains HeaderIDs

[PATCH] D125468: [clangd] Include Cleaner: ignore headers with IWYU export pragmas

2022-05-12 Thread Kirill Bobyrev via Phabricator via cfe-commits 
kbobyrev updated this revision to Diff 428947.
kbobyrev added a comment.

Remove unwanted formatting changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125468

Files:
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.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
@@ -544,8 +544,7 @@
   EXPECT_TRUE(
   ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, RecursiveInclusion) {
@@ -576,8 +575,35 @@
   findReferencedLocations(AST), AST.getIncludeStructure(),
   AST.getCanonicalIncludes(), AST.getSourceManager());
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+}
+
+TEST(IncludeCleaner, IWYUPragmaExport) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "foo.h"
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#ifndef FOO_H
+#define FOO_H
+
+#include "bar.h" // IWYU pragma: export
+
+#endif
+  )cpp";
+  TU.AdditionalFiles["bar.h"] = guard(R"cpp(
+void bar() {}
+  )cpp");
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles = findReferencedFiles(
+  findReferencedLocations(AST), AST.getIncludeStructure(),
+  AST.getCanonicalIncludes(), AST.getSourceManager());
+  EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
+  // FIXME: This is not correct: foo.h and bar.h are unused but are not
+  // diagnosed as such because we ignore headers with IWYU export pragmas for
+  // now.
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -406,7 +406,7 @@
   #define RECURSIVE_H
 
   #include "recursive.h"
-  
+
   #endif // RECURSIVE_H
 )cpp";
 
@@ -418,6 +418,32 @@
   EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
 }
 
+TEST_F(HeadersTest, HasIWYUPragmas) {
+  FS.Files[MainFile] = R"cpp(
+#include "export.h"
+#include "begin_exports.h"
+#include "none.h"
+)cpp";
+  FS.Files[testPath("export.h")] = R"cpp(
+#pragma once
+#include "none.h" // IWYU pragma: export
+)cpp";
+  FS.Files[testPath("begin_exports.h")] = R"cpp(
+#pragma once
+// IWYU pragma: begin_exports
+#include "none.h"
+// IWYU pragma: end_exports
+)cpp";
+  FS.Files[testPath("none.h")] = R"cpp(
+#pragma once
+)cpp";
+
+  auto Includes = collectIncludes();
+  EXPECT_TRUE(Includes.hasIWYUPragmas(getID("export.h", Includes)));
+  EXPECT_TRUE(Includes.hasIWYUPragmas(getID("begin_exports.h", Includes)));
+  EXPECT_FALSE(Includes.hasIWYUPragmas(getID("none.h", Includes)));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -271,9 +271,13 @@
   // Headers without include guards have side effects and are not
   // self-contained, skip them.
   assert(Inc.HeaderID);
+  auto HID = static_cast(*Inc.HeaderID);
+  // FIXME: Ignore the headers with IWYU export pragmas for now, remove this
+  // check when we have actual support for export pragmas.
+  if (AST.getIncludeStructure().hasIWYUPragmas(HID))
+return false;
   auto FE = AST.getSourceManager().getFileManager().getFileRef(
-  AST.getIncludeStructure().getRealPath(
-  static_cast(*Inc.HeaderID)));
+  AST.getIncludeStructure().getRealPath(HID));
   assert(FE);
   if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded(
   >getFileEntry())) {
Index: clang-tools-extra/clangd/Headers.h
===
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -145,6 +145,10 @@
 return !NonSelfContained.contains(ID);
   }
 
+  bool hasIWYUPragmas(HeaderID ID) const {
+return HasIWYUPragmas.contains(ID);
+  }
+
   // Return all transitively reachable files.
   llvm::ArrayRef allHeaders() const { return RealPathNames; }
 
@@ -185,6 +189,9 @@
   // Contains

[PATCH] D125468: [clangd] Include Cleaner: ignore headers with IWYU export pragmas

2022-05-12 Thread Kirill Bobyrev via Phabricator via cfe-commits 
kbobyrev created this revision.
kbobyrev added reviewers: sammccall, kadircet.
Herald added subscribers: usaxena95, arphaman.
Herald added a project: All.
kbobyrev requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Disable the warnings with `IWYU pragma: export` or `begin_exports` +
`end_exports` until we have support for these pragmas. There are too many
false-positive warnings for the headers that have the correct pragmas for now
and it makes the user experience very unpleasant.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125468

Files:
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.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
@@ -544,8 +544,7 @@
   EXPECT_TRUE(
   ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, RecursiveInclusion) {
@@ -576,8 +575,35 @@
   findReferencedLocations(AST), AST.getIncludeStructure(),
   AST.getCanonicalIncludes(), AST.getSourceManager());
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+}
+
+TEST(IncludeCleaner, IWYUPragmaExport) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "foo.h"
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#ifndef FOO_H
+#define FOO_H
+
+#include "bar.h" // IWYU pragma: export
+
+#endif
+  )cpp";
+  TU.AdditionalFiles["bar.h"] = guard(R"cpp(
+void bar() {}
+  )cpp");
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles = findReferencedFiles(
+  findReferencedLocations(AST), AST.getIncludeStructure(),
+  AST.getCanonicalIncludes(), AST.getSourceManager());
+  EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
+  // FIXME: This is not correct: foo.h and bar.h are unused but are not
+  // diagnosed as such because we ignore headers with IWYU export pragmas for
+  // now.
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -406,7 +406,7 @@
   #define RECURSIVE_H
 
   #include "recursive.h"
-  
+
   #endif // RECURSIVE_H
 )cpp";
 
@@ -418,6 +418,32 @@
   EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
 }
 
+TEST_F(HeadersTest, HasIWYUPragmas) {
+  FS.Files[MainFile] = R"cpp(
+#include "export.h"
+#include "begin_exports.h"
+#include "none.h"
+)cpp";
+  FS.Files[testPath("export.h")] = R"cpp(
+#pragma once
+#include "none.h" // IWYU pragma: export
+)cpp";
+  FS.Files[testPath("begin_exports.h")] = R"cpp(
+#pragma once
+// IWYU pragma: begin_exports
+#include "none.h"
+// IWYU pragma: end_exports
+)cpp";
+  FS.Files[testPath("none.h")] = R"cpp(
+#pragma once
+)cpp";
+
+  auto Includes = collectIncludes();
+  EXPECT_TRUE(Includes.hasIWYUPragmas(getID("export.h", Includes)));
+  EXPECT_TRUE(Includes.hasIWYUPragmas(getID("begin_exports.h", Includes)));
+  EXPECT_FALSE(Includes.hasIWYUPragmas(getID("none.h", Includes)));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -82,7 +82,7 @@
 // Using templateName case is handled by the override TraverseTemplateName.
 if (TST->getTemplateName().getKind() == TemplateName::UsingTemplate)
   return true;
-add(TST->getAsCXXRecordDecl());  // Specialization
+add(TST->getAsCXXRecordDecl()); // Specialization
 return true;
   }
 
@@ -271,9 +271,13 @@
   // Headers without include guards have side effects and are not
   // self-contained, skip them.
   assert(Inc.HeaderID);
+  auto HID = static_cast(*Inc.HeaderID);
+  // FIXME: Ignore the headers with IWYU export pragmas for now, remove this
+  // check when we have actual support for export pragmas.
+  if (AST.getIncludeStructure().hasIWYUPragmas(HID))
+return false;
   auto FE = AST.getSourceManager().getFileManager().getFileRef(
-