kadircet created this revision.
kadircet added a reviewer: hokein.
Herald added a subscriber: arphaman.
Herald added a project: All.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147139

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


Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -384,6 +384,35 @@
   EXPECT_THAT(Findings.UnusedIncludes, IsEmpty());
 }
 
+TEST(IncludeCleaner, MacroExpandedThroughIncludes) {
+  Annotations MainFile(R"cpp(
+  #include "all.h"
+  #define FOO(X) const Foo *X
+  void foo() {
+  #include [["expander.inc"]]
+  }
+)cpp");
+
+  TestTU TU;
+  TU.AdditionalFiles["expander.inc"] = guard("FOO(f1);FOO(f2);");
+  TU.AdditionalFiles["foo.h"] = guard("struct Foo {};");
+  TU.AdditionalFiles["all.h"] = guard("#include \"foo.h\"");
+
+  TU.Code = MainFile.code();
+  ParsedAST AST = TU.build();
+
+  auto Findings = computeIncludeCleanerFindings(AST).MissingIncludes;
+  // FIXME: Deduplicate references resulting from expansion of the same macro 
in
+  // multiple places.
+  EXPECT_THAT(Findings, testing::SizeIs(2));
+  auto RefRange = Findings.front().SymRefRange;
+  auto &SM = AST.getSourceManager();
+  EXPECT_EQ(RefRange.file(), SM.getMainFileID());
+  // FIXME: Point at the spelling location, rather than the include.
+  EXPECT_EQ(halfOpenToRange(SM, RefRange.toCharRange(SM)), MainFile.range());
+  EXPECT_EQ(RefRange, Findings[1].SymRefRange);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -51,9 +51,11 @@
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include <cassert>
 #include <iterator>
 #include <optional>
 #include <string>
+#include <utility>
 #include <vector>
 
 namespace clang {
@@ -266,7 +268,6 @@
 }
 } // namespace
 
-
 std::vector<include_cleaner::SymbolReference>
 collectMacroReferences(ParsedAST &AST) {
   const auto &SM = AST.getSourceManager();
@@ -398,6 +399,10 @@
         // FIXME: Use presumed locations to map such usages back to patched
         // locations safely.
         auto Loc = SM.getFileLoc(Ref.RefLocation);
+        // File locations can be outside of the main file if macro is expanded
+        // through an #include.
+        while (SM.getFileID(Loc) != SM.getMainFileID())
+          Loc = SM.getIncludeLoc(SM.getFileID(Loc));
         const auto *Token = AST.getTokens().spelledTokenAt(Loc);
         MissingIncludeDiagInfo DiagInfo{Ref.Target, Token->range(SM),
                                         Providers};


Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -384,6 +384,35 @@
   EXPECT_THAT(Findings.UnusedIncludes, IsEmpty());
 }
 
+TEST(IncludeCleaner, MacroExpandedThroughIncludes) {
+  Annotations MainFile(R"cpp(
+  #include "all.h"
+  #define FOO(X) const Foo *X
+  void foo() {
+  #include [["expander.inc"]]
+  }
+)cpp");
+
+  TestTU TU;
+  TU.AdditionalFiles["expander.inc"] = guard("FOO(f1);FOO(f2);");
+  TU.AdditionalFiles["foo.h"] = guard("struct Foo {};");
+  TU.AdditionalFiles["all.h"] = guard("#include \"foo.h\"");
+
+  TU.Code = MainFile.code();
+  ParsedAST AST = TU.build();
+
+  auto Findings = computeIncludeCleanerFindings(AST).MissingIncludes;
+  // FIXME: Deduplicate references resulting from expansion of the same macro in
+  // multiple places.
+  EXPECT_THAT(Findings, testing::SizeIs(2));
+  auto RefRange = Findings.front().SymRefRange;
+  auto &SM = AST.getSourceManager();
+  EXPECT_EQ(RefRange.file(), SM.getMainFileID());
+  // FIXME: Point at the spelling location, rather than the include.
+  EXPECT_EQ(halfOpenToRange(SM, RefRange.toCharRange(SM)), MainFile.range());
+  EXPECT_EQ(RefRange, Findings[1].SymRefRange);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -51,9 +51,11 @@
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include <cassert>
 #include <iterator>
 #include <optional>
 #include <string>
+#include <utility>
 #include <vector>
 
 namespace clang {
@@ -266,7 +268,6 @@
 }
 } // namespace
 
-
 std::vector<include_cleaner::SymbolReference>
 collectMacroReferences(ParsedAST &AST) {
   const auto &SM = AST.getSourceManager();
@@ -398,6 +399,10 @@
         // FIXME: Use presumed locations to map such usages back to patched
         // locations safely.
         auto Loc = SM.getFileLoc(Ref.RefLocation);
+        // File locations can be outside of the main file if macro is expanded
+        // through an #include.
+        while (SM.getFileID(Loc) != SM.getMainFileID())
+          Loc = SM.getIncludeLoc(SM.getFileID(Loc));
         const auto *Token = AST.getTokens().spelledTokenAt(Loc);
         MissingIncludeDiagInfo DiagInfo{Ref.Target, Token->range(SM),
                                         Providers};
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D147139: [clangd] ... Kadir Cetinkaya via Phabricator via cfe-commits

Reply via email to