[PATCH] D94500: Rework Whitesmiths mode to use line-level values in UnwrappedLineParser

[Marek Kurdej via Phabricator via cfe-commits]

curdeius requested changes to this revision.
curdeius added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:613
+  // settings. This allows the format to back up one level in those cases.
+  if (!AddLevel && NamespaceBlock &&
+      Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
----------------
HazardyKnusperkeks wrote:
> Maybe give `!AddLevel && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths` a name? You check it three times.
Agree on this.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2159
+
+    // If we're in whitesmiths mode, indent the brace.
+    if (!AddLevel && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
----------------
Nit: please write consistently Whitesmiths with a capital W.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2163
+
+    parseBlock(/*MustBeDeclaration=*/true, AddLevel, true, true);
     // Munch the semicolon after a namespace. This is more common than one would
----------------
3*true? Please add comments a minima.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2259
+  // If in Whitesmiths mode, the line with the while() needs to be indented
+  // to the same level as the block
+  if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
----------------
Nit: full stop at the end of the phrase.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2967
+  // needs to be indented.
+  bool ClosesBlock =
+      Line->MatchingOpeningBlockLineIndex != UnwrappedLine::kInvalidIndex &&
----------------
Please move below to the place of use. Also, just naming it `ClosesBlock` is a bit misleading as it is only for Whitesmiths style.


================
Comment at: clang/unittests/Format/FormatTest.cpp:13688
 
-  WhitesmithsBraceStyle.IndentCaseBlocks = true;
+  WhitesmithsBraceStyle.IndentCaseLabels = true;
   verifyFormat("void switchTest1(int a)\n"
----------------
Hmm, you don't test the same thing anymore...


================
Comment at: clang/unittests/Format/FormatTest.cpp:13696
                "      }\n"
-               "    break;\n"
+               "      break;\n"
                "    }\n"
----------------
MyDeveloperDay wrote:
> whilst I don't really like changing the indentation I don't think there was anything that says it was correct before.
That is really strange that `case` is indented differently than `break`. Is it intended? Is the style really like this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94500/new/

https://reviews.llvm.org/D94500