[PATCH] D96744: clang-format IncludeBlocks: Regroup determination of "main" for framework-style includes fix

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc created this revision.
poelmanc added reviewers: MyDeveloperDay, hokein, sammccall.
poelmanc added projects: clang-tools-extra, clang-format.
poelmanc requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Most modern libraries and applications include files with relative paths from a root (e.g. `#include <foo/bar/baz.h>` versus `#include "baz.h"`.) When regrouping, a file's "main include" should be left at the top (given priority 0.) The existing `IncludeCategoryManager::isMainHeader()` checks only the file //stem// (e.g. `baz.h`), and fails to identify main includes with angle brackets despite a comment saying `// remove the surrounding "" or <>`.

This patch fixes these issues by comparing all directory components present in both the `#include` string and the file name, and by allowing angle bracket includes to be considered "main". It adds a new `IsMainHeader` unit test to check behavior of framework-style includes, which would fail without this patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96744

Files:
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
  clang/unittests/Tooling/HeaderIncludesTest.cpp


Index: clang/unittests/Tooling/HeaderIncludesTest.cpp
===================================================================
--- clang/unittests/Tooling/HeaderIncludesTest.cpp
+++ clang/unittests/Tooling/HeaderIncludesTest.cpp
@@ -91,6 +91,28 @@
   EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
 }
 
+TEST_F(HeaderIncludesTest, IsMainHeader) {
+  Style = format::getGoogleStyle(format::FormatStyle::LanguageKind::LK_Cpp)
+              .IncludeStyle;
+  std::vector<StringRef> FileNames{"foo/bar/baz.cpp", "foo/bar/baz.cu.cpp",
+                                   "foo/bar/baz_test.cu.cpp"};
+  for (const StringRef &FileName : FileNames) {
+    IncludeCategoryManager Manager(Style, FileName);
+    // These framework-style includes should all be considered "main".
+    EXPECT_EQ(Manager.getIncludePriority("<foo/bar/baz.h>", true), 0)
+        << "for source file " << FileName;
+    EXPECT_EQ(Manager.getIncludePriority("\"bar/baz.h\"", true), 0)
+        << "for source file " << FileName;
+    EXPECT_EQ(Manager.getIncludePriority("<baz.h>", true), 0)
+        << "for source file " << FileName;
+    // These should not be considered "main" as the paths to baz.h differ.
+    EXPECT_NE(Manager.getIncludePriority("<bar/foo/baz.h>", true), 0)
+        << "for source file " << FileName;
+    EXPECT_NE(Manager.getIncludePriority("\"foo/baz.h\"", true), 0)
+        << "for source file " << FileName;
+  }
+}
+
 TEST_F(HeaderIncludesTest, InsertAfterMainHeader) {
   std::string Code = "#include \"fix.h\"\n"
                      "\n"
Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===================================================================
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -233,9 +233,6 @@
   return Ret;
 }
 bool IncludeCategoryManager::isMainHeader(StringRef IncludeName) const {
-  if (!IncludeName.startswith("\""))
-    return false;
-
   IncludeName =
       IncludeName.drop_front(1).drop_back(1); // remove the surrounding "" or <>
   // Not matchingStem: implementation files may have compound extensions but
@@ -259,8 +256,22 @@
   if (!Matching.empty()) {
     llvm::Regex MainIncludeRegex(HeaderStem.str() + Style.IncludeIsMainRegex,
                                  llvm::Regex::IgnoreCase);
-    if (MainIncludeRegex.match(Matching))
+    if (MainIncludeRegex.match(Matching)) {
+      // Matching is non-empty so these should be non-empty as well, making
+      // ++rbegin(IncludeName) and ++rbegin(FileName) safe.
+      assert(!IncludeName.empty());
+      assert(!FileName.empty());
+      // Checked stems above. Check remaining common path components here.
+      auto IncludePathRIter = ++llvm::sys::path::rbegin(IncludeName);
+      auto FilePathRiter = ++llvm::sys::path::rbegin(FileName);
+      for (; IncludePathRIter != llvm::sys::path::rend(IncludeName) &&
+             FilePathRiter != llvm::sys::path::rend(FileName);
+           ++IncludePathRIter, ++FilePathRiter) {
+        if (*IncludePathRIter != *FilePathRiter)
+          return false;
+      }
       return true;
+    }
   }
   return false;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D96744.323865.patch
Type: text/x-patch
Size: 3133 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210216/aa9edb22/attachment.bin>