diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp --- a/clang/lib/Format/Format.cpp +++ b/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 Matches; SmallVector 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) { diff --git a/clang/lib/Tooling/Inclusions/HeaderIncludes.cpp b/clang/lib/Tooling/Inclusions/HeaderIncludes.cpp --- a/clang/lib/Tooling/Inclusions/HeaderIncludes.cpp +++ b/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 #include 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::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::max(); } + bool IncludeCategoryManager::isMainHeader(StringRef IncludeName) const { if (!IncludeName.startswith("\"")) return false; diff --git a/clang/lib/Tooling/Inclusions/IncludeStyle.cpp b/clang/lib/Tooling/Inclusions/IncludeStyle.cpp --- a/clang/lib/Tooling/Inclusions/IncludeStyle.cpp +++ b/clang/lib/Tooling/Inclusions/IncludeStyle.cpp @@ -7,17 +7,26 @@ //===----------------------------------------------------------------------===// #include "clang/Tooling/Inclusions/IncludeStyle.h" +#include 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::max(); + void MappingTraits::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); } diff --git a/clang/unittests/Format/SortIncludesTest.cpp b/clang/unittests/Format/SortIncludesTest.cpp --- a/clang/unittests/Format/SortIncludesTest.cpp +++ b/clang/unittests/Format/SortIncludesTest.cpp @@ -8,6 +8,7 @@ #include "FormatTestUtils.h" #include "clang/Format/Format.h" +#include "clang/Tooling/Inclusions/IncludeStyle.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Debug.h" #include "gtest/gtest.h" @@ -535,16 +536,48 @@ "#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", + sort("#include \"b.h\"\n" + "#include \"a.h\"\n" + "#include \"c.h\"\n" + "#include \"llvm/a.h\"\n", + "a.cc")); + + // Keep manually splitted groups in place EXPECT_EQ("#include \"a.h\"\n" "#include \"b.h\"\n" "#include \"c.h\"\n" + "// no main:\n" "#include \"llvm/a.h\"\n", sort("#include \"b.h\"\n" "#include \"a.h\"\n" "#include \"c.h\"\n" + "// no main:\n" "#include \"llvm/a.h\"\n", "a.cc")); + + // Keep both main files together (with priority 0) + Style.IncludeIsMainRegex = "([-_](test|unittest))?$"; + EXPECT_EQ("#include \"a.h\"\n" + "#include \"a_test.h\"\n" + "#include \"b.h\"\n" + "#include \"c.h\"\n", + sort("#include \"b.h\"\n" + "#include \"a.h\"\n" + "#include \"c.h\"\n" + "#include \"a_test.h\"\n", + "a_test.cc")); } TEST_F(SortIncludesTest, LeavesMainHeaderFirstInAdditionalExtensions) { @@ -762,6 +795,64 @@ "#include \"c_main.h\"\n" "#include \"a_other.h\"\n", "c_main.cc", 0)); + // All negative + Style.IncludeCategories = {{".*important_os_header.*", -3, 0, false}, + {".*", -2, 0, false}}; + + EXPECT_EQ("#include \"important_os_header.h\"\n" + "#include \"a_other.h\"\n" + "#include \"c_main.h\"\n", + sort("#include \"c_main.h\"\n" + "#include \"a_other.h\"\n" + "#include \"important_os_header.h\"\n", + "c_main.cc")); +} + +TEST_F(SortIncludesTest, ZeroPriorities) { + Style.IncludeCategories = {{".*important_os_header.*", 0, -1, false}, + {".*", 1, 0, false}}; + EXPECT_EQ("#include \"important_os_header.h\"\n" + "#include \"c_main.h\"\n" + "#include \"a_other.h\"\n", + sort("#include \"c_main.h\"\n" + "#include \"a_other.h\"\n" + "#include \"important_os_header.h\"\n", + "c_main.cc")); + + // check stable when re-run + EXPECT_EQ("#include \"important_os_header.h\"\n" + "#include \"c_main.h\"\n" + "#include \"a_other.h\"\n", + sort("#include \"important_os_header.h\"\n" + "#include \"c_main.h\"\n" + "#include \"a_other.h\"\n", + "c_main.cc", 0)); + + // All zero, sorted after name + Style.IncludeCategories = {{".*important_os_header.*", 0, 0, false}, + {".*", 0, 0, false}}; + + EXPECT_EQ("#include \"a_other.h\"\n" + "#include \"c_main.h\"\n" + "#include \"important_os_header.h\"\n", + sort("#include \"important_os_header.h\"\n" + "#include \"c_main.h\"\n" + "#include \"a_other.h\"\n", + "c_main.cc")); + + // Grouped: + Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup; + Style.IncludeCategories = {{".*important_os_header.*", 0, -1, false}, + {".*", 1, 0, false}}; + + EXPECT_EQ("#include \"important_os_header.h\"\n" + "#include \"c_main.h\"\n" + "\n" + "#include \"a_other.h\"\n", + sort("#include \"a_other.h\"\n" + "#include \"c_main.h\"\n" + "#include \"important_os_header.h\"\n", + "c_main.cc")); } TEST_F(SortIncludesTest, PriorityGroupsAreSeparatedWhenRegroupping) {