HazardyKnusperkeks requested changes to this revision. HazardyKnusperkeks added a comment. This revision now requires changes to proceed.
Only some small changes, then it looks good to me. And I will definitely use it, once it is landed. ================ Comment at: clang/include/clang/Format/Format.h:3056 + SDS_Leave, + /// Insert empty lines between definition blocks. + SDS_Always, ---------------- Right? ================ Comment at: clang/include/clang/Format/Format.h:4097 + ArrayRef<tooling::Range> Ranges, + StringRef FileName = "<stdin>"); + ---------------- The only use of this function I found is in the tests and it sets the argument, or am I mistaken? ================ Comment at: clang/lib/Format/DefinitionBlockSeparator.cpp:33-34 + SmallVectorImpl<AnnotatedLine *> &Lines, tooling::Replacements &Result) { + if (Style.SeparateDefinitionBlocks == FormatStyle::SDS_Leave) + return; + auto likelyDefinition = [this](AnnotatedLine *Line) { ---------------- It should never be instantiated with that. ================ Comment at: clang/lib/Format/DefinitionBlockSeparator.cpp:35 + return; + auto likelyDefinition = [this](AnnotatedLine *Line) { + if (Line->MightBeFunctionDecl && Line->mightBeFunctionDefinition()) ---------------- ================ Comment at: clang/lib/Format/DefinitionBlockSeparator.cpp:59 + for (unsigned I = 0; I < Lines.size(); I++) { + auto Line = Lines[I]; + FormatToken *TargetToken = nullptr; ---------------- ================ Comment at: clang/lib/Format/DefinitionBlockSeparator.cpp:63 + auto OpeningLineIndex = Line->MatchingOpeningBlockLineIndex; + const auto insertReplacement = [&](int NewlineToInsert) { + assert(TargetLine); ---------------- ================ Comment at: clang/lib/Format/DefinitionBlockSeparator.cpp:76 + }; + const auto followingOtherOpening = [&]() { + return OpeningLineIndex == 0 || ---------------- ================ Comment at: clang/lib/Format/DefinitionBlockSeparator.cpp:80 + }; + const auto hasEnumOnLine = [Line]() { + FormatToken *CurrentToken = Line->First; ---------------- ================ Comment at: clang/lib/Format/DefinitionBlockSeparator.cpp:93 + if (hasEnumOnLine()) { + // We have no scope opening/closing information for enum + IsDefBlock = 1; ---------------- ================ Comment at: clang/lib/Format/DefinitionBlockSeparator.cpp:128 + + // Not the last token + if (IsDefBlock && I + 1 < Lines.size()) { ---------------- ================ Comment at: clang/lib/Format/Format.cpp:455 +template <> +struct ScalarEnumerationTraits<FormatStyle::SeparateDefinitionStyle> { + static void enumeration(IO &IO, FormatStyle::SeparateDefinitionStyle &Value) { ---------------- Please try to find a better place, regarding sorting (yeah the are some mismatches). ================ Comment at: clang/lib/Format/Format.cpp:783 IO.mapOptional("ShortNamespaceLines", Style.ShortNamespaceLines); + IO.mapOptional("SeparateDefinitionBlocks", Style.SeparateDefinitionBlocks); IO.mapOptional("SortIncludes", Style.SortIncludes); ---------------- Please sort before ShortNamespaceLines ================ Comment at: clang/lib/Format/Format.cpp:3059 + if (Style.SeparateDefinitionBlocks) + Passes.emplace_back([&](const Environment &Env) { ---------------- ================ Comment at: clang/lib/Format/WhitespaceManager.h:48 + /// Infers whether the input is using CRLF + static bool inputUsesCRLF(StringRef Text, bool DefaultToCRLF); ---------------- CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116314/new/ https://reviews.llvm.org/D116314 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits