Hm, seems to me that this is broken either way. If config.h remains first, that is good, but the main #include is unlikely to remain second. I think we should give the main #include a non-zero #include category and then properly configure config.h to be before that. I'll try to get to that this week.
On Sun, Oct 18, 2015 at 6:40 PM, Nico Weber <tha...@chromium.org> wrote: > On Tue, Sep 29, 2015 at 12:53 AM, Daniel Jasper via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: djasper >> Date: Tue Sep 29 02:53:08 2015 >> New Revision: 248782 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=248782&view=rev >> Log: >> clang-format: Extend #include sorting functionality >> >> Recognize the main module header as well as different #include categories. >> This should now mimic the behavior of llvm/utils/sort_includes.py as >> well as clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.cpp very >> closely. >> >> Modified: >> cfe/trunk/include/clang/Format/Format.h >> cfe/trunk/lib/Format/Format.cpp >> cfe/trunk/tools/clang-format/ClangFormat.cpp >> cfe/trunk/unittests/Format/SortIncludesTest.cpp >> >> Modified: cfe/trunk/include/clang/Format/Format.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Format/Format.h?rev=248782&r1=248781&r2=248782&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/Format/Format.h (original) >> +++ cfe/trunk/include/clang/Format/Format.h Tue Sep 29 02:53:08 2015 >> @@ -259,6 +259,21 @@ struct FormatStyle { >> /// For example: BOOST_FOREACH. >> std::vector<std::string> ForEachMacros; >> >> + /// \brief Regular expressions denoting the different #include >> categories used >> + /// for ordering #includes. >> + /// >> + /// These regular expressions are matched against the filename of an >> include >> + /// (including the <> or "") in order. The value belonging to the first >> + /// matching regular expression is assigned and #includes are sorted >> first >> + /// according to increasing category number and then alphabetically >> within >> + /// each category. >> + /// >> + /// If none of the regular expressions match, UINT_MAX is assigned as >> + /// category. The main header for a source file automatically gets >> category 0, >> + /// so that it is kept at the beginning of the #includes >> + /// (http://llvm.org/docs/CodingStandards.html#include-style). >> + std::vector<std::pair<std::string, unsigned>> IncludeCategories; >> + >> /// \brief Indent case labels one level from the switch statement. >> /// >> /// When \c false, use the same indentation level as for the switch >> statement. >> >> Modified: cfe/trunk/lib/Format/Format.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=248782&r1=248781&r2=248782&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Format/Format.cpp (original) >> +++ cfe/trunk/lib/Format/Format.cpp Tue Sep 29 02:53:08 2015 >> @@ -13,6 +13,7 @@ >> /// >> >> >> //===----------------------------------------------------------------------===// >> >> +#include "clang/Format/Format.h" >> #include "ContinuationIndenter.h" >> #include "TokenAnnotator.h" >> #include "UnwrappedLineFormatter.h" >> @@ -21,7 +22,6 @@ >> #include "clang/Basic/Diagnostic.h" >> #include "clang/Basic/DiagnosticOptions.h" >> #include "clang/Basic/SourceManager.h" >> -#include "clang/Format/Format.h" >> #include "clang/Lex/Lexer.h" >> #include "llvm/ADT/STLExtras.h" >> #include "llvm/Support/Allocator.h" >> @@ -375,6 +375,9 @@ FormatStyle getLLVMStyle() { >> LLVMStyle.ForEachMacros.push_back("foreach"); >> LLVMStyle.ForEachMacros.push_back("Q_FOREACH"); >> LLVMStyle.ForEachMacros.push_back("BOOST_FOREACH"); >> + LLVMStyle.IncludeCategories = {{"^\"(llvm|llvm-c|clang|clang-c)/", 2}, >> + {"^(<|\"(gtest|isl|json)/)", 3}, >> + {".*", 1}}; >> LLVMStyle.IndentCaseLabels = false; >> LLVMStyle.IndentWrappedFunctionNames = false; >> LLVMStyle.IndentWidth = 2; >> @@ -423,6 +426,7 @@ FormatStyle getGoogleStyle(FormatStyle:: >> GoogleStyle.AlwaysBreakTemplateDeclarations = true; >> GoogleStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = true; >> GoogleStyle.DerivePointerAlignment = true; >> + GoogleStyle.IncludeCategories = {{"^<.*\\.h>", 1}, {"^<.*", 2}, {".*", >> 3}}; >> GoogleStyle.IndentCaseLabels = true; >> GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false; >> GoogleStyle.ObjCSpaceAfterProperty = false; >> @@ -1575,7 +1579,7 @@ struct IncludeDirective { >> StringRef Filename; >> StringRef Text; >> unsigned Offset; >> - bool IsAngled; >> + unsigned Category; >> }; >> >> } // end anonymous namespace >> @@ -1605,7 +1609,8 @@ static void sortIncludes(const FormatSty >> for (unsigned i = 0, e = Includes.size(); i != e; ++i) >> Indices.push_back(i); >> std::sort(Indices.begin(), Indices.end(), [&](unsigned LHSI, unsigned >> RHSI) { >> - return Includes[LHSI].Filename < Includes[RHSI].Filename; >> + return std::tie(Includes[LHSI].Category, Includes[LHSI].Filename) < >> + std::tie(Includes[RHSI].Category, Includes[RHSI].Filename); >> }); >> >> // If the #includes are out of order, we generate a single replacement >> fixing >> @@ -1642,22 +1647,49 @@ tooling::Replacements sortIncludes(const >> tooling::Replacements Replaces; >> unsigned Prev = 0; >> unsigned SearchFrom = 0; >> - llvm::Regex IncludeRegex(R"(^[\t\ ]*#[\t\ >> ]*include[^"<]*["<]([^">]*)([">]))"); >> + llvm::Regex IncludeRegex( >> + R"(^[\t\ ]*#[\t\ ]*include[^"<]*(["<][^">]*[">]))"); >> 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 sanity checking, e.g. edit distance of the base >> name, to fix >> + // cases where the first #include is unlikely to be the main header. >> > > FYI: This happens to make include sorting in webkit cpp files work too, > where the style guide says that one must include config.h first and the > main header second (https://www.webkit.org/coding/coding-style.html). > Since clang-format won't reorder the first line, it keeps the config.h > header first, even though config.h isn't technically the main header. If > this FIXME gets fixed, it'd be good if formatting of webkit cpp files isn't > broke in the process. > > >> + bool LookForMainHeader = FileName.endswith(".c") || >> + FileName.endswith(".cc") || >> + FileName.endswith(".cpp")|| >> + FileName.endswith(".c++")|| >> + FileName.endswith(".cxx"); >> + >> + // Create pre-compiled regular expressions for the #include categories. >> + SmallVector<llvm::Regex, 4> CategoryRegexs; >> + for (const auto &IncludeBlock : Style.IncludeCategories) >> + CategoryRegexs.emplace_back(IncludeBlock.first); >> + >> for (;;) { >> auto Pos = Code.find('\n', SearchFrom); >> StringRef Line = >> Code.substr(Prev, (Pos != StringRef::npos ? Pos : Code.size()) - >> Prev); >> if (!Line.endswith("\\")) { >> if (IncludeRegex.match(Line, &Matches)) { >> - bool IsAngled = Matches[2] == ">"; >> - if (!IncludesInBlock.empty() && >> - IsAngled != IncludesInBlock.back().IsAngled) { >> - sortIncludes(Style, IncludesInBlock, Ranges, FileName, >> Replaces); >> - IncludesInBlock.clear(); >> + unsigned Category; >> + if (LookForMainHeader && !Matches[1].startswith("<")) { >> + Category = 0; >> + } else { >> + Category = UINT_MAX; >> + for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i) { >> + if (CategoryRegexs[i].match(Matches[1])) { >> + Category = Style.IncludeCategories[i].second; >> + break; >> + } >> + } >> } >> - IncludesInBlock.push_back({Matches[1], Line, Prev, Matches[2] == >> ">"}); >> + LookForMainHeader = false; >> + IncludesInBlock.push_back({Matches[1], Line, Prev, Category}); >> } else if (!IncludesInBlock.empty()) { >> sortIncludes(Style, IncludesInBlock, Ranges, FileName, Replaces); >> IncludesInBlock.clear(); >> >> Modified: cfe/trunk/tools/clang-format/ClangFormat.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-format/ClangFormat.cpp?rev=248782&r1=248781&r2=248782&view=diff >> >> ============================================================================== >> --- cfe/trunk/tools/clang-format/ClangFormat.cpp (original) >> +++ cfe/trunk/tools/clang-format/ClangFormat.cpp Tue Sep 29 02:53:08 2015 >> @@ -73,7 +73,7 @@ FallbackStyle("fallback-style", >> cl::init("LLVM"), cl::cat(ClangFormatCategory)); >> >> static cl::opt<std::string> >> -AssumeFilename("assume-filename", >> +AssumeFileName("assume-filename", >> cl::desc("When reading from stdin, clang-format assumes >> this\n" >> "filename to look for a style config file >> (with\n" >> "-style=file) and to determine the language."), >> @@ -239,13 +239,13 @@ static bool format(StringRef FileName) { >> std::vector<tooling::Range> Ranges; >> if (fillRanges(Code.get(), Ranges)) >> return true; >> - FormatStyle FormatStyle = getStyle( >> - Style, (FileName == "-") ? AssumeFilename : FileName, >> FallbackStyle); >> + StringRef AssumedFileName = (FileName == "-") ? AssumeFileName : >> FileName; >> + FormatStyle FormatStyle = getStyle(Style, AssumedFileName, >> FallbackStyle); >> Replacements Replaces; >> std::string ChangedCode; >> if (SortIncludes) { >> Replaces = >> - sortIncludes(FormatStyle, Code->getBuffer(), Ranges, FileName); >> + sortIncludes(FormatStyle, Code->getBuffer(), Ranges, >> AssumedFileName); >> ChangedCode = tooling::applyAllReplacements(Code->getBuffer(), >> Replaces); >> for (const auto &R : Replaces) >> Ranges.push_back({R.getOffset(), R.getLength()}); >> @@ -324,7 +324,7 @@ int main(int argc, const char **argv) { >> if (DumpConfig) { >> std::string Config = >> clang::format::configurationAsText(clang::format::getStyle( >> - Style, FileNames.empty() ? AssumeFilename : FileNames[0], >> + Style, FileNames.empty() ? AssumeFileName : FileNames[0], >> FallbackStyle)); >> llvm::outs() << Config << "\n"; >> return 0; >> >> Modified: cfe/trunk/unittests/Format/SortIncludesTest.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/SortIncludesTest.cpp?rev=248782&r1=248781&r2=248782&view=diff >> >> ============================================================================== >> --- cfe/trunk/unittests/Format/SortIncludesTest.cpp (original) >> +++ cfe/trunk/unittests/Format/SortIncludesTest.cpp Tue Sep 29 02:53:08 >> 2015 >> @@ -20,13 +20,15 @@ namespace { >> >> class SortIncludesTest : public ::testing::Test { >> protected: >> - std::string sort(llvm::StringRef Code) { >> + std::string sort(llvm::StringRef Code, StringRef FileName = >> "input.cpp") { >> std::vector<tooling::Range> Ranges(1, tooling::Range(0, >> Code.size())); >> - std::string Sorted = applyAllReplacements( >> - Code, sortIncludes(getLLVMStyle(), Code, Ranges, "input.cpp")); >> - return applyAllReplacements( >> - Sorted, reformat(getLLVMStyle(), Sorted, Ranges, "input.cpp")); >> + std::string Sorted = >> + applyAllReplacements(Code, sortIncludes(Style, Code, Ranges, >> FileName)); >> + return applyAllReplacements(Sorted, >> + reformat(Style, Sorted, Ranges, >> FileName)); >> } >> + >> + FormatStyle Style = getLLVMStyle(); >> }; >> >> TEST_F(SortIncludesTest, BasicSorting) { >> @@ -76,13 +78,23 @@ TEST_F(SortIncludesTest, SortsLocallyInE >> "#include \"c.h\"\n" >> "\n" >> "#include \"b.h\"\n", >> - sort("#include \"c.h\"\n" >> - "#include \"a.h\"\n" >> + sort("#include \"a.h\"\n" >> + "#include \"c.h\"\n" >> "\n" >> "#include \"b.h\"\n")); >> } >> >> TEST_F(SortIncludesTest, HandlesAngledIncludesAsSeparateBlocks) { >> + EXPECT_EQ("#include \"a.h\"\n" >> + "#include \"c.h\"\n" >> + "#include <b.h>\n" >> + "#include <d.h>\n", >> + sort("#include <d.h>\n" >> + "#include <b.h>\n" >> + "#include \"c.h\"\n" >> + "#include \"a.h\"\n")); >> + >> + Style = getGoogleStyle(FormatStyle::LK_Cpp); >> EXPECT_EQ("#include <b.h>\n" >> "#include <d.h>\n" >> "#include \"a.h\"\n" >> @@ -103,6 +115,24 @@ TEST_F(SortIncludesTest, HandlesMultilin >> "#include \"b.h\"\n")); >> } >> >> +TEST_F(SortIncludesTest, LeavesMainHeaderFirst) { >> + EXPECT_EQ("#include \"llvm/a.h\"\n" >> + "#include \"b.h\"\n" >> + "#include \"c.h\"\n", >> + sort("#include \"llvm/a.h\"\n" >> + "#include \"c.h\"\n" >> + "#include \"b.h\"\n")); >> + >> + // Don't do this in headers. >> + EXPECT_EQ("#include \"b.h\"\n" >> + "#include \"c.h\"\n" >> + "#include \"llvm/a.h\"\n", >> + sort("#include \"llvm/a.h\"\n" >> + "#include \"c.h\"\n" >> + "#include \"b.h\"\n", >> + "some_header.h")); >> +} >> + >> } // end namespace >> } // end namespace format >> } // end namespace clang >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
