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

[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 <system_header.h>
 
@@ -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<IncludeStructure::HeaderID>(*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 =


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D112695.382970.patch
Type: text/x-patch
Size: 3410 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211028/4a483ba8/attachment.bin>