Febbe updated this revision to Diff 496647.
Febbe added a comment.
Fixed the Unit Tests
- rewritten one test, which made the assumption, that there can be only one
main header.
- it now asserts, that all matching headers are considered as main header.
- replaced initialisations of `IncludeCategories.SortPriority` to zero with
the value from `IncludeCategories.Priority`
- the previous approach to set the SortPriority when it's 0 to Priority,
instead of initializing it directly as intended had several drawbacks:
- values of 0 were not possible and resulted in weird behav.
- when a main include was matched by another matcher, it got annother
sortpriority than 0.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143691/new/
https://reviews.llvm.org/D143691
Files:
clang/lib/Format/Format.cpp
clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
clang/lib/Tooling/Inclusions/IncludeStyle.cpp
clang/unittests/Format/SortIncludesTest.cpp
Index: clang/unittests/Format/SortIncludesTest.cpp
===================================================================
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -87,7 +87,7 @@
TEST_F(SortIncludesTest, SortedIncludesUsingSortPriorityAttribute) {
FmtStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
FmtStyle.IncludeStyle.IncludeCategories = {
- {"^<sys/param\\.h>", 1, 0, false},
+ {"^<sys/param\\.h>", 1, 1, false},
{"^<sys/types\\.h>", 1, 1, false},
{"^<sys.*/", 1, 2, false},
{"^<uvm/", 2, 3, false},
@@ -535,11 +535,18 @@
"#include \"b.h\"\n",
"a.cc"));
- // Only recognize the first #include with a matching basename as main include.
+ // Todo (remove-before-merge): I consider the assumption, that there is only
+ // one main include as wrong.
+ // E.g. a.cpp -> a.priv.h && a.h
+ // E.g. a_test.cpp -> a_test.h && a.h
+ // Maybe add a "//clang-format pragma: not_main" to remove false positives
+
+ // Recognize all possible main #include's with a matching basename as main
+ // include.
EXPECT_EQ("#include \"a.h\"\n"
+ "#include \"llvm/a.h\"\n"
"#include \"b.h\"\n"
- "#include \"c.h\"\n"
- "#include \"llvm/a.h\"\n",
+ "#include \"c.h\"\n",
sort("#include \"b.h\"\n"
"#include \"a.h\"\n"
"#include \"c.h\"\n"
@@ -645,8 +652,9 @@
"a.h"));
Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
- Style.IncludeCategories = {
- {"^\"", 1, 0, false}, {"^<.*\\.h>$", 2, 0, false}, {"^<", 3, 0, false}};
+ Style.IncludeCategories = {{"^\"", 1, 1, false}, //
+ {"^<.*\\.h>$", 2, 2, false},
+ {"^<", 3, 3, false}};
StringRef UnsortedCode = "#include \"qt.h\"\n"
"#include <algorithm>\n"
@@ -695,11 +703,11 @@
TEST_F(SortIncludesTest, SupportOptionalCaseSensitiveMachting) {
Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
- Style.IncludeCategories = {{"^\"", 1, 0, false},
- {"^<.*\\.h>$", 2, 0, false},
- {"^<Q[A-Z][^\\.]*>", 3, 0, false},
- {"^<Qt[^\\.]*>", 4, 0, false},
- {"^<", 5, 0, false}};
+ Style.IncludeCategories = {{"^\"", 1, 1, false},
+ {"^<.*\\.h>$", 2, 2, false},
+ {"^<Q[A-Z][^\\.]*>", 3, 3, false},
+ {"^<Qt[^\\.]*>", 4, 4, false},
+ {"^<", 5, 5, false}};
StringRef UnsortedCode = "#include <QWidget>\n"
"#include \"qt.h\"\n"
@@ -744,8 +752,8 @@
}
TEST_F(SortIncludesTest, NegativePriorities) {
- Style.IncludeCategories = {{".*important_os_header.*", -1, 0, false},
- {".*", 1, 0, false}};
+ Style.IncludeCategories = {{".*important_os_header.*", -1, -1, false},
+ {".*", 1, 1, false}};
EXPECT_EQ("#include \"important_os_header.h\"\n"
"#include \"c_main.h\"\n"
"#include \"a_other.h\"\n",
@@ -765,8 +773,8 @@
}
TEST_F(SortIncludesTest, PriorityGroupsAreSeparatedWhenRegroupping) {
- Style.IncludeCategories = {{".*important_os_header.*", -1, 0, false},
- {".*", 1, 0, false}};
+ Style.IncludeCategories = {{".*important_os_header.*", -1, -1, false},
+ {".*", 1, 1, false}};
Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
EXPECT_EQ("#include \"important_os_header.h\"\n"
Index: clang/lib/Tooling/Inclusions/IncludeStyle.cpp
===================================================================
--- clang/lib/Tooling/Inclusions/IncludeStyle.cpp
+++ clang/lib/Tooling/Inclusions/IncludeStyle.cpp
@@ -15,9 +15,9 @@
void MappingTraits<IncludeStyle::IncludeCategory>::mapping(
IO &IO, IncludeStyle::IncludeCategory &Category) {
- IO.mapOptional("Regex", Category.Regex);
- IO.mapOptional("Priority", Category.Priority);
- IO.mapOptional("SortPriority", Category.SortPriority);
+ IO.mapRequired("Regex", Category.Regex);
+ IO.mapRequired("Priority", Category.Priority);
+ IO.mapOptional("SortPriority", Category.SortPriority, Category.Priority);
IO.mapOptional("CaseSensitive", Category.RegexIsCaseSensitive);
}
Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===================================================================
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -206,33 +206,35 @@
}
}
+constexpr int DefaultMainIncludePriority = 0;
+constexpr int DefaultMainIncludeSortPriority = 0;
+
int IncludeCategoryManager::getIncludePriority(StringRef IncludeName,
bool CheckMainHeader) const {
- int Ret = INT_MAX;
+ if (CheckMainHeader && IsMainFile && isMainHeader(IncludeName))
+ return DefaultMainIncludePriority;
+
for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i)
if (CategoryRegexs[i].match(IncludeName)) {
- Ret = Style.IncludeCategories[i].Priority;
- break;
+ return Style.IncludeCategories[i].Priority;
}
- if (CheckMainHeader && IsMainFile && Ret > 0 && isMainHeader(IncludeName))
- Ret = 0;
- return Ret;
+
+ return std::numeric_limits<int>::max();
}
int IncludeCategoryManager::getSortIncludePriority(StringRef IncludeName,
bool CheckMainHeader) const {
- int Ret = INT_MAX;
+ if (CheckMainHeader && IsMainFile && isMainHeader(IncludeName))
+ return DefaultMainIncludeSortPriority;
+
for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i)
if (CategoryRegexs[i].match(IncludeName)) {
- Ret = Style.IncludeCategories[i].SortPriority;
- if (Ret == 0)
- Ret = Style.IncludeCategories[i].Priority;
- break;
+ return Style.IncludeCategories[i].SortPriority;
}
- if (CheckMainHeader && IsMainFile && Ret > 0 && isMainHeader(IncludeName))
- Ret = 0;
- return Ret;
+
+ return std::numeric_limits<int>::max();
}
+
bool IncludeCategoryManager::isMainHeader(StringRef IncludeName) const {
if (!IncludeName.startswith("\""))
return false;
Index: clang/lib/Format/Format.cpp
===================================================================
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1376,9 +1376,9 @@
LLVMStyle.ForEachMacros.push_back("BOOST_FOREACH");
LLVMStyle.IfMacros.push_back("KJ_IF_MAYBE");
LLVMStyle.IncludeStyle.IncludeCategories = {
- {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 0, false},
- {"^(<|\"(gtest|gmock|isl|json)/)", 3, 0, false},
- {".*", 1, 0, false}};
+ {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 2, false},
+ {"^(<|\"(gtest|gmock|isl|json)/)", 3, 3, false},
+ {".*", 1, 1, false}};
LLVMStyle.IncludeStyle.IncludeIsMainRegex = "(Test)?$";
LLVMStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve;
LLVMStyle.IndentAccessModifiers = false;
@@ -1506,10 +1506,10 @@
GoogleStyle.AlwaysBreakBeforeMultilineStrings = true;
GoogleStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_Yes;
GoogleStyle.DerivePointerAlignment = true;
- GoogleStyle.IncludeStyle.IncludeCategories = {{"^<ext/.*\\.h>", 2, 0, false},
- {"^<.*\\.h>", 1, 0, false},
- {"^<.*", 2, 0, false},
- {".*", 3, 0, false}};
+ GoogleStyle.IncludeStyle.IncludeCategories = {{"^<ext/.*\\.h>", 2, 2, false},
+ {"^<.*\\.h>", 1, 1, false},
+ {"^<.*", 2, 2, false},
+ {".*", 3, 3, false}};
GoogleStyle.IncludeStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
GoogleStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
GoogleStyle.IndentCaseLabels = true;
@@ -2966,16 +2966,12 @@
SmallVector<StringRef, 4> Matches;
SmallVector<IncludeDirective, 16> IncludesInBlock;
- // In compiled files, consider the first #include to be the main #include of
- // the file if it is not a system #include. This ensures that the header
- // doesn't have hidden dependencies
- // (http://llvm.org/docs/CodingStandards.html#include-style).
- //
// FIXME: Do some validation, e.g. edit distance of the base name, to fix
// cases where the first #include is unlikely to be the main header.
tooling::IncludeCategoryManager Categories(Style.IncludeStyle, FileName);
bool FirstIncludeBlock = true;
- bool MainIncludeFound = false;
+ // Todo(remove before merge): removed: multiple files can be main includes
+ // This option caused random sorting, depending which was detected first.
bool FormattingOff = false;
// '[' must be the first and '-' the last character inside [...].
@@ -3029,11 +3025,11 @@
}
int Category = Categories.getIncludePriority(
IncludeName,
- /*CheckMainHeader=*/!MainIncludeFound && FirstIncludeBlock);
- int Priority = Categories.getSortIncludePriority(
- IncludeName, !MainIncludeFound && FirstIncludeBlock);
- if (Category == 0)
- MainIncludeFound = true;
+ /*CheckMainHeader=*/FirstIncludeBlock);
+ int Priority =
+ Categories.getSortIncludePriority(IncludeName, FirstIncludeBlock);
+ // Todo(remove before merge): reason for removal: every header can have
+ // 0,0 priority
IncludesInBlock.push_back(
{IncludeName, Line, Prev, Category, Priority});
} else if (!IncludesInBlock.empty() && !EmptyLineSkipped) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
