On Mon, Oct 19, 2015 at 4:24 AM, Daniel Jasper <djas...@google.com> wrote:
> 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. > In practice, the first two lines are a block with two includes: config.h first and the main header seconds. Since clang-format doesn't move the first line around and since it does include-block-internal sorting only, it currently never reorders this first two-includes block. > 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. > Sounds great, thanks! > > 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
