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;
}
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;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
