Febbe updated this revision to Diff 497687.
Febbe marked 3 inline comments as done.
Febbe added a comment.
- Removed some, not required changes.
- Reverted mapOptional -> mapRequired change, since it might break existing
configs
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
@@ -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"
Index: clang/lib/Tooling/Inclusions/IncludeStyle.cpp
===================================================================
--- clang/lib/Tooling/Inclusions/IncludeStyle.cpp
+++ clang/lib/Tooling/Inclusions/IncludeStyle.cpp
@@ -7,17 +7,26 @@
//===----------------------------------------------------------------------===//
#include "clang/Tooling/Inclusions/IncludeStyle.h"
+#include <limits>
using clang::tooling::IncludeStyle;
namespace llvm {
namespace yaml {
+// Todo (remove before merge): changes here are required,
+// because the explicit override for the default values of 0 are moved from
+// the algorithm to this place
+// Since we can't make those values required, we must set the previously
+// intended default values here, to prevent behaviour changes.
+
+constexpr int DefaultPriority = std::numeric_limits<int>::max();
+
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.mapOptional("Priority", Category.Priority, DefaultPriority);
+ 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,33 @@
}
}
+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;
- }
- if (CheckMainHeader && IsMainFile && Ret > 0 && isMainHeader(IncludeName))
- Ret = 0;
- return Ret;
+ if (CategoryRegexs[i].match(IncludeName))
+ return Style.IncludeCategories[i].Priority;
+
+ 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;
- }
- if (CheckMainHeader && IsMainFile && Ret > 0 && isMainHeader(IncludeName))
- Ret = 0;
- return Ret;
+ if (CategoryRegexs[i].match(IncludeName))
+ return Style.IncludeCategories[i].SortPriority;
+
+ 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
@@ -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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
