Febbe updated this revision to Diff 496650.
Febbe added a comment.
Added priority to the sorting algorithm.
This prevents unwanted splits and weird resorts of groups with the same
priority, but with different and overlapping (other groups) SortPriority.
The new system can now be understood as Primary.SecondaryPriority instead of:
sort by SecondPriority + bucket By PrimaryPriority
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
@@ -12,6 +12,7 @@
#include "clang/Lex/Lexer.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Path.h"
+#include <limits>
#include <optional>
namespace clang {
@@ -206,33 +207,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;
@@ -2875,15 +2875,17 @@
llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
const auto LHSFilenameLower = Includes[LHSI].Filename.lower();
const auto RHSFilenameLower = Includes[RHSI].Filename.lower();
- return std::tie(Includes[LHSI].Priority, LHSFilenameLower,
- Includes[LHSI].Filename) <
- std::tie(Includes[RHSI].Priority, RHSFilenameLower,
- Includes[RHSI].Filename);
+ return std::tie(Includes[LHSI].Category, Includes[LHSI].Priority,
+ LHSFilenameLower, Includes[LHSI].Filename) <
+ std::tie(Includes[RHSI].Category, Includes[RHSI].Priority,
+ RHSFilenameLower, Includes[RHSI].Filename);
});
} else {
llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
- return std::tie(Includes[LHSI].Priority, Includes[LHSI].Filename) <
- std::tie(Includes[RHSI].Priority, Includes[RHSI].Filename);
+ return std::tie(Includes[LHSI].Category, Includes[LHSI].Priority,
+ Includes[LHSI].Filename) <
+ std::tie(Includes[RHSI].Category, Includes[RHSI].Priority,
+ Includes[RHSI].Filename);
});
}
@@ -2966,16 +2968,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 +3027,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
