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<IncludeStructure::HeaderID>(*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<IncludeStructure::HeaderID>(*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(
           &FE->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 hasIWYUExport(HeaderID ID) const {
+    return HasIWYUPragmas.contains(ID);
+  }
+
   // Return all transitively reachable files.
   llvm::ArrayRef<std::string> allHeaders() const { return RealPathNames; }
 
@@ -185,6 +189,9 @@
   // Contains HeaderIDs of all non self-contained entries in the
   // IncludeStructure.
   llvm::DenseSet<HeaderID> NonSelfContained;
+  // Contains a set of headers that have either "IWYU pragma: export" or "IWYU
+  // pragma: begin_exports".
+  llvm::DenseSet<HeaderID> HasIWYUPragmas;
 };
 
 // Calculates insertion edit for including a new header in a file.
Index: clang-tools-extra/clangd/Headers.cpp
===================================================================
--- clang-tools-extra/clangd/Headers.cpp
+++ clang-tools-extra/clangd/Headers.cpp
@@ -24,6 +24,7 @@
 
 const char IWYUPragmaKeep[] = "// IWYU pragma: keep";
 const char IWYUPragmaExport[] = "// IWYU pragma: export";
+const char IWYUPragmaBeginExports[] = "// IWYU pragma: begin_exports";
 
 class IncludeStructure::RecordHeaders : public PPCallbacks,
                                         public CommentHandler {
@@ -127,31 +128,45 @@
     }
   }
 
-  // Given:
-  //
-  // #include "foo.h"
-  // #include "bar.h" // IWYU pragma: keep
-  //
-  // The order in which the callbacks will be triggered:
-  //
-  // 1. InclusionDirective("foo.h")
-  // 2. HandleComment("// IWYU pragma: keep")
-  // 3. InclusionDirective("bar.h")
-  //
-  // HandleComment will store the last location of "IWYU pragma: keep" (or
-  // export) comment in the main file, so that when InclusionDirective is
-  // called, it will know that the next inclusion is behind the IWYU pragma.
   bool HandleComment(Preprocessor &PP, SourceRange Range) override {
-    if (!inMainFile() || Range.getBegin().isMacroID())
-      return false;
     bool Err = false;
     llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), &Err);
-    if (Err && !Text.consume_front(IWYUPragmaKeep) &&
-        !Text.consume_front(IWYUPragmaExport))
+    if (Err)
       return false;
-    unsigned Offset = SM.getFileOffset(Range.getBegin());
-    LastPragmaKeepInMainFileLine =
-        SM.getLineNumber(SM.getFileID(Range.getBegin()), Offset) - 1;
+    if (inMainFile()) {
+      // Given:
+      //
+      // #include "foo.h"
+      // #include "bar.h" // IWYU pragma: keep
+      //
+      // The order in which the callbacks will be triggered:
+      //
+      // 1. InclusionDirective("foo.h")
+      // 2. handleCommentInMainFile("// IWYU pragma: keep")
+      // 3. InclusionDirective("bar.h")
+      //
+      // This code stores the last location of "IWYU pragma: keep" (or export)
+      // comment in the main file, so that when InclusionDirective is called, it
+      // will know that the next inclusion is behind the IWYU pragma.
+      // FIXME: Support "IWYU pragma: begin_exports" and "IWYU pragma:
+      // end_exports".
+      if (!Text.startswith(IWYUPragmaExport) &&
+          !Text.startswith(IWYUPragmaKeep))
+        return false;
+      unsigned Offset = SM.getFileOffset(Range.getBegin());
+      LastPragmaKeepInMainFileLine =
+          SM.getLineNumber(SM.getMainFileID(), Offset) - 1;
+    } else {
+      // Memorize headers that that have export pragmas in them. Include Cleaner
+      // does not support them properly yet, so they will be not marked as
+      // unused.
+      // FIXME: Once IncludeCleaner supports export pragmas, remove this.
+      if (!Text.startswith(IWYUPragmaExport) &&
+          !Text.startswith(IWYUPragmaBeginExports))
+        return false;
+      Out->HasIWYUPragmas.insert(
+          *Out->getID(SM.getFileEntryForID(SM.getFileID(Range.getBegin()))));
+    }
     return false;
   }
 
@@ -237,8 +252,7 @@
   return It->second;
 }
 
-IncludeStructure::HeaderID
-IncludeStructure::getOrCreateID(FileEntryRef Entry) {
+IncludeStructure::HeaderID IncludeStructure::getOrCreateID(FileEntryRef Entry) {
   // Main file's FileEntry was not known at IncludeStructure creation time.
   if (&Entry.getFileEntry() == MainFileEntry) {
     if (RealPathNames.front().empty())
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to