[PATCH] D116314: [clang-format] Add style to separate definition blocks

[Björn Schäpers via Phabricator via cfe-commits]

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