[clang-format] Rework Whitesmiths mode to use line-level values in UnwrappedLineParser

[timwoj]

Summary

This commit removes the old way of handling Whitesmiths mode in favor of just setting the
levels during parsing and letting the formatter handle it from there. It requires a bit of
special-casing during the parsing, but ends up a bit cleaner than before. It also removes
some of switch/case unit tests that don't really make much sense when dealing with
Whitesmiths.

[timwoj]

This fixes https://bugs.llvm.org/show_bug.cgi?id=48569, amongst other things.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

13707

any reason why this is being removed?

[HazardyKnusperkeks]

clang/lib/Format/TokenAnalyzer.cpp

71 ↗

(On Diff #316099)

Unrelated change (although I think it's good one).

clang/lib/Format/UnwrappedLineParser.h

143–148

A bool parameter for something like that is not very nice. An enum (class) is much nicer.

If one does not know the declaration addUnwrappedLine(false) seems a bit odd, addUnwrappedLine(LineLevel::Keep) looks better. (The naming may be changed.)

clang/unittests/Format/FormatTest.cpp

13528

So until now it has formatted that always wrong?

[timwoj]

clang/lib/Format/TokenAnalyzer.cpp

71 ↗

(On Diff #316099)

Should I split that into a separate commit?

clang/unittests/Format/FormatTest.cpp

13528

Hm, that's a good question. I'll have to think about that one. I guess for Whitesmiths and it's always wanting to indent braces, this should be fixed back to way that it was.

13707

This is an artifact of there not being any sort of actual guide for Whitesmiths. I have it set now to always indent the case labels (and it should just ignore the IndentCaseLabels option entirely, like it does for IndentCaseBlocks). I can certainly fix that option to work again though.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnalyzer.cpp

71 ↗

(On Diff #316099)

In my Opinion that would be the best.

[MyDeveloperDay]

clang/lib/Format/Format.cpp

819 ↗

(On Diff #316099)

I guess its fine to define these defaults here but what if people turn them off in their config what will happen?

clang/unittests/Format/FormatTest.cpp

13707

see the images from D93806: [clang-format] PR48569 clang-format fails to align case label with `switch` with Whitesmith Indentation, you are saying that anyone with bracewrappingsytle of Whitesmith cannot decide if they can or cannot indent case labels?

Could you show me a spec that show Whitesmiths is the other way around?

[timwoj]

clang/lib/Format/UnwrappedLineParser.h

143–148

Noted, I'll make it an enum in the next update.

clang/unittests/Format/FormatTest.cpp

13707

Part of the problem is that there isn't any sort of official spec for Whitesmiths that I've been able to find. I'm working on a project that uses it, and have been basically going off their layout as the proper way of doing things. Looking at those images, I agree that the option should continue to function.

[timwoj]

Updates from review:

• Fixed handling of IndentCaseLabels
• Fixed indentation of namespace braces
• Replaced bool argument to addUnwrappedLine with an enum

[timwoj]

Fixing diff to look at the right commit

[HazardyKnusperkeks]

Is there a test with indentation within a namespace? If not maybe you should add one.

clang/lib/Format/UnwrappedLineParser.cpp

613

Maybe give !AddLevel && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths a name? You check it three times.

clang/lib/Format/UnwrappedLineParser.h

146

I would prefer an enum class.

[timwoj]

Add tests for Whitesmiths with all of the NamespaceIndentation options

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

13548

you are removing the test with 1 namespace nesting

[timwoj]

Restored lost single-nesting namespace test

[MyDeveloperDay]

LGTM I think, it would be good to get input from other reviewers

clang/unittests/Format/FormatTest.cpp

13612

whilst I don't really like changing the indentation I don't think there was anything that says it was correct before.

[curdeius]

clang/lib/Format/UnwrappedLineParser.cpp

613

Agree on this.

2159

Nit: please write consistently Whitesmiths with a capital W.

2163

3*true? Please add comments a minima.

2259

Nit: full stop at the end of the phrase.

2967

Please move below to the place of use. Also, just naming it ClosesBlock is a bit misleading as it is only for Whitesmiths style.

clang/unittests/Format/FormatTest.cpp

13604

Hmm, you don't test the same thing anymore...

13612

That is really strange that case is indented differently than break. Is it intended? Is the style really like this?

[curdeius]

clang/unittests/Format/FormatTest.cpp

13612

Forget it. I was thinking about something else.

[timwoj]

clang/lib/Format/UnwrappedLineParser.cpp

2967

I agree with renaming it, but I'm not sure on moving it to where it's used. It relies on Line->MatchingOpeningBlockLineIndex which is reset shortly after this boolean value is calculated.

clang/unittests/Format/FormatTest.cpp

13604

This change was intentional. According to the clang-format documentation, IndentCaseBlocks effectively just changes the brace breaking for the block after a case label. Given that it always breaks for Whitesmiths, IndentCaseBlocks can be effectively ignored for that style. I changed it to test the IndentCaseLabels instead because that tests indentation for the whole block.

[timwoj]

• Make LineLevel an enum class
• Combine multiple duplicate checks into a named variable
• Minor formatting fixes

[curdeius]

LGTM. Thanks for working on this.

clang/lib/Format/UnwrappedLineParser.cpp

2967

Right. I've overseen it. It's good now.

[timwoj]

No problem, sorry it took a bit of back-and-forth there. Is there any way this can sneak into 12.0?

[curdeius]

I don't think it can go into 12.x. But if you have a good reason and a related bug report you might add this bug ticket as a release blocker of https://llvm.org/PR48902.

BTW, would you mind updating clang/docs/ReleaseNotes.rst?

[timwoj]

https://bugs.llvm.org/show_bug.cgi?id=48668 already exists as a bug report linked to this review. I'm not sure "the Zeek project would like to start using this" is a good enough reason to block an LLVM release though. I'm fairly certain there aren't a whole lot of people in the world using Whitesmiths. That said, there's already at least one clang-format bug in the release-blocker list.

I'll get the release notes updated shortly.

[MyDeveloperDay]

My advice leave it out of the release, the next release comes round pretty quickly. This gives us 6 months of people who use the snapshots to iron out any issues.

[timwoj]

Added release note entry

[HazardyKnusperkeks]

Have you rebased your change? It seems that your release notes are still for LLVM 12.

[timwoj]

Rebased on main

[HazardyKnusperkeks]

Do you need someone to push this?

[timwoj]

In D94500#2592448, @HazardyKnusperkeks wrote:

Do you need someone to push this?

Yes I do. I don't have committer access.

[HazardyKnusperkeks]

In D94500#2593229, @timwoj wrote:

In D94500#2592448, @HazardyKnusperkeks wrote:

Do you need someone to push this?

Yes I do. I don't have committer access.

Please state the name and mail for the commit.

[timwoj]

In D94500#2594394, @HazardyKnusperkeks wrote:

In D94500#2593229, @timwoj wrote:

In D94500#2592448, @HazardyKnusperkeks wrote:

Do you need someone to push this?

Yes I do. I don't have committer access.

Please state the name and mail for the commit.

Tim Wojtulewicz
[e-mail]

[HazardyKnusperkeks]

Could you please rebase your patch an reupload the diff, it doesn't apply anymore. (And doing it by hand is too much work for me.)

[timwoj]

Rebase onto master again

[HazardyKnusperkeks]

In D94500#2604754, @timwoj wrote:

Rebase onto master again

master or main? Please rebase on main. That is where the pushes land. (And as far as I can see master hasn't been updated in quite some time.

[timwoj]

In D94500#2605515, @HazardyKnusperkeks wrote:

In D94500#2604754, @timwoj wrote:

Rebase onto master again

master or main? Please rebase on main. That is where the pushes land. (And as far as I can see master hasn't been updated in quite some time.

Yes, main, sorry. It was late in my work day and I was fried already.

[HazardyKnusperkeks]

In D94500#2605517, @timwoj wrote:

In D94500#2605515, @HazardyKnusperkeks wrote:

In D94500#2604754, @timwoj wrote:

Rebase onto master again

master or main? Please rebase on main. That is where the pushes land. (And as far as I can see master hasn't been updated in quite some time.

Yes, main, sorry. It was late in my work day and I was fried already.

No problem.

[MyDeveloperDay]

A crash (https://bugs.llvm.org/show_bug.cgi?id=50663) in 12.0.1 is fixed by your changes here when merged to the 12 branch, I'm not sure if this needs to be backported to 12 I guess it might depend on if we think it's critical enough or if there will more 12 release candidates before 13 @timwoj any thoughts?

[timwoj]

A crash (https://bugs.llvm.org/show_bug.cgi?id=50663) in 12.0.1 is fixed by your changes here when merged to the 12 branch, I'm not sure if this needs to be backported to 12 I guess it might depend on if we think it's critical enough or if there will more 12 release candidates before 13 @timwoj any thoughts?

It should apply cleanly to 12.0 as far as I know, since if I remember right I had asked if it could be merged before 12.0 was released. I don't have a whole lot of free cycles at the moment to help with it if it needs a lot of changes though. My vote is yes to backport it, if it counts for anything.

[MyDeveloperDay]

I think we've missed the deadline for 12.0.1 which I believe was last friday. I did a practice merge and it didn't quite go in cleanly to 12.0.X branch

Normally we would mark the bug in the bug tracker for the next release, this is what I did for another bug and @tstellar I think picked it up and merge it for us.

I'm not sure if we are doing a 12.0.2 release but I've added that as the "Blocks" tag in the bug tracker, one of us can merge it if Tom wants us to.

To be honest looking at the timelines

I'd probably say can it just wait for 13.0

[MyDeveloperDay]

This has been landed in the 12.0.1 RC https://github.com/llvm/llvm-project/commit/c7d7ace46258b04aa4b5df08952bfebc6fc4ce94

clang/lib/Format/UnwrappedLineFormatter.cpp

void UnwrappedLineFormatter::formatFirstToken(
   // Remove empty lines after access specifiers.
   if (PreviousLine && PreviousLine->First->isAccessSpecifier() &&
       (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))
     Newlines = std::min(1u, Newlines);
 
   if (Newlines)
     Indent = NewlineIndent;
 
-  // If in Whitemsmiths mode, indent start and end of blocks
-  if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) {
-    if (RootToken.isOneOf(tok::l_brace, tok::r_brace, tok::kw_case,
-                          tok::kw_default))
-      Indent += Style.IndentWidth;
-  }
-
   // Preprocessor directives get indented before the hash only if specified
   if (Line.Type == LT_PreprocessorDirective ||
       Line.Type == LT_ImportStatement) {
     switch (Style.IndentPPDirectives) {
     case FormatStyle::PPDIS_AfterHash:
       Indent = 0;
       break;
     case FormatStyle::PPDIS_None:

clang/lib/Format/UnwrappedLineParser.h

public:
 
   void parse();
 
 private:
   void reset();
   void parseFile();
   void parseLevel(bool HasOpeningBrace);
   void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
-                  bool MunchSemi = true);
+                  bool MunchSemi = true, bool NamespaceBlock = false);
   void parseChildBlock();
   void parsePPDirective(unsigned Level);
   void parsePPDefine();
   void parsePPIf(bool IfDef);
   void parsePPElIf();
   void parsePPElse();
   void parsePPEndIf();
   void parsePPUnknown();
private:
   // See:
   // https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/where-generic-type-constraint
   void parseCSharpGenericTypeConstraint();
   bool tryToParseLambda();
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
   bool tryToParseSimpleAttribute();
-  void addUnwrappedLine();
+
+  // Used by addUnwrappedLine to denote whether to keep or remove a level
+  // when resetting the line state.
+  enum LineLevel { LL_Remove, LL_Keep };
+
+  void addUnwrappedLine(LineLevel AdjustLevel = LineLevel::LL_Remove);
   bool eof() const;
   // LevelDifference is the difference of levels after and before the current
   // token. For example:
   // - if the token is '{' and opens a block, LevelDifference is 1.
   // - if the token is '}' and closes a block, LevelDifference is -1.
   void nextToken(int LevelDifference = 0);
   void readToken(int LevelDifference = 0);
 

clang/lib/Format/UnwrappedLineParser.cpp

size_t UnwrappedLineParser::computePPHash() const {
   for (const auto &i : PPStack) {
     hash_combine(h, size_t(i.Kind));
     hash_combine(h, i.Line);
   }
   return h;
 }
 
 void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
-                                     bool MunchSemi) {
+                                     bool MunchSemi, bool NamespaceBlock) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
          "'{' or macro block token expected");
   const bool MacroBlock = FormatTok->is(TT_MacroBlockBegin);
   FormatTok->setBlockKind(BK_Block);
 
+  // For Whitesmiths mode, jump to the next level prior to skipping over the
+  // braces.
+  if (AddLevel && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
+    ++Line->Level;
+
   size_t PPStartHash = computePPHash();
 
   unsigned InitialLevel = Line->Level;
   nextToken(/*LevelDifference=*/AddLevel ? 1 : 0);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
   size_t NbPreprocessorDirectives =
       CurrentLines == &Lines ? PreprocessorDirectives.size() : 0;
   addUnwrappedLine();
   size_t OpeningLineIndex =
       CurrentLines->empty()
           ? (UnwrappedLine::kInvalidIndex)
           : (CurrentLines->size() - 1 - NbPreprocessorDirectives);
 
+  // Whitesmiths is weird here. The brace needs to be indented for the namespace
+  // block, but the block itself may not be indented depending on the style
+  // settings. This allows the format to back up one level in those cases.
+  if (!AddLevel && NamespaceBlock &&
+      Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
+    --Line->Level;
+
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
                                           MustBeDeclaration);
-  if (AddLevel)
+  if (AddLevel && Style.BreakBeforeBraces != FormatStyle::BS_Whitesmiths)
     ++Line->Level;
   parseLevel(/*HasOpeningBrace=*/true);
 
   if (eof())
     return;
 
   if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
                  : !FormatTok->is(tok::r_brace)) {
if (FormatTok->is(tok::arrow)) {
     nextToken();
     parseStructuralElement();
   }
 
   if (MunchSemi && FormatTok->Tok.is(tok::semi))
     nextToken();
 
   Line->Level = InitialLevel;
+  FormatTok->setBlockKind(BK_Block);
 
   if (PPStartHash == PPEndHash) {
     Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
     if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) {
       // Update the opening line to add the forward reference as well
       (*CurrentLines)[OpeningLineIndex].MatchingClosingBlockLineIndex =
           CurrentLines->size() - 1;
     }
void UnwrappedLineParser::parseNamespace() {
   }
   if (FormatTok->Tok.is(tok::l_brace)) {
     if (ShouldBreakBeforeBrace(Style, InitialToken))
       addUnwrappedLine();
 
     bool AddLevel = Style.NamespaceIndentation == FormatStyle::NI_All ||
                     (Style.NamespaceIndentation == FormatStyle::NI_Inner &&
                      DeclarationScopeStack.size() > 1);
-    parseBlock(/*MustBeDeclaration=*/true, AddLevel);
+
+    // If we're in whitesmiths mode, indent the brace.
+    if (!AddLevel && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
+      ++Line->Level;
+
+    parseBlock(/*MustBeDeclaration=*/true, AddLevel, true, true);
     // Munch the semicolon after a namespace. This is more common than one would
     // think. Putting the semicolon into its own line is very ugly.
     if (FormatTok->Tok.is(tok::semi))
       nextToken();
-    addUnwrappedLine();
+
+    addUnwrappedLine(AddLevel ? LineLevel::LL_Remove : LineLevel::LL_Keep);
+
+    if (!AddLevel && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
+      --Line->Level;
   }
   // FIXME: Add error handling.
 }
 
 void UnwrappedLineParser::parseNew() {
   assert(FormatTok->is(tok::kw_new) && "'new' expected");
   nextToken();
 
void UnwrappedLineParser::parseDoWhile() {
   }
 
   // FIXME: Add error handling.
   if (!FormatTok->Tok.is(tok::kw_while)) {
     addUnwrappedLine();
     return;
   }
 
+  // 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)
+    ++Line->Level;
+
   nextToken();
   parseStructuralElement();
 }
 
 void UnwrappedLineParser::parseLabel(bool LeftAlignLabel) {
   nextToken();
   unsigned OldLineLevel = Line->Level;
   if (Line->Level > 1 || (!Line->InPPDirective && Line->Level > 0))
     --Line->Level;
   if (LeftAlignLabel)
     Line->Level = 0;
 
-  bool RemoveWhitesmithsCaseIndent =
-      (!Style.IndentCaseBlocks &&
-       Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths);
-
-  if (RemoveWhitesmithsCaseIndent)
-    --Line->Level;
-
   if (!Style.IndentCaseBlocks && CommentsBeforeNextToken.empty() &&
       FormatTok->Tok.is(tok::l_brace)) {
 
-    CompoundStatementIndenter Indenter(
-        this, Line->Level, Style.BraceWrapping.AfterCaseLabel,
-        Style.BraceWrapping.IndentBraces || RemoveWhitesmithsCaseIndent);
+    CompoundStatementIndenter Indenter(this, Line->Level,
+                                       Style.BraceWrapping.AfterCaseLabel,
+                                       Style.BraceWrapping.IndentBraces);
     parseBlock(/*MustBeDeclaration=*/false);
     if (FormatTok->Tok.is(tok::kw_break)) {
       if (Style.BraceWrapping.AfterControlStatement ==
           FormatStyle::BWACS_Always) {
         addUnwrappedLine();
-        if (RemoveWhitesmithsCaseIndent) {
+        if (!Style.IndentCaseBlocks &&
+            Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) {
           Line->Level++;
         }
       }
       parseStructuralElement();
     }
     addUnwrappedLine();
   } else {
     if (FormatTok->is(tok::semi))
for (SmallVectorImpl<UnwrappedLine>::const_iterator
              E = Node.Children.end();
          I != E; ++I) {
       printDebugInfo(*I, "\nChild: ");
     }
   }
   llvm::dbgs() << "\n";
 }
 
-void UnwrappedLineParser::addUnwrappedLine() {
+void UnwrappedLineParser::addUnwrappedLine(LineLevel AdjustLevel) {
   if (Line->Tokens.empty())
     return;
   LLVM_DEBUG({
     if (CurrentLines == &Lines)
       printDebugInfo(*Line);
   });
+
+  // If this line closes a block when in Whitesmiths mode, remember that
+  // information so that the level can be decreased after the line is added.
+  // This has to happen after the addition of the line since the line itself
+  // needs to be indented.
+  bool ClosesBlock =
+      Line->MatchingOpeningBlockLineIndex != UnwrappedLine::kInvalidIndex &&
+      Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths;
+
   CurrentLines->push_back(std::move(*Line));
   Line->Tokens.clear();
   Line->MatchingOpeningBlockLineIndex = UnwrappedLine::kInvalidIndex;
   Line->FirstStartColumn = 0;
+
+  if (ClosesBlock && AdjustLevel == LineLevel::LL_Remove)
+    --Line->Level;
   if (CurrentLines == &Lines && !PreprocessorDirectives.empty()) {
     CurrentLines->append(
         std::make_move_iterator(PreprocessorDirectives.begin()),
         std::make_move_iterator(PreprocessorDirectives.end()));
     PreprocessorDirectives.clear();
   }
   // Disconnect the current token from the last token on the previous line.
   FormatTok->Previous = nullptr;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, WhitesmithsBraceBreaking) {
   WhitesmithsBraceStyle.ColumnLimit = 0;
 
   // FIXME: this test case can't decide whether there should be a blank line
   // after the ~D() line or not. It adds one if one doesn't exist in the test
   // and it removes the line if one exists.
   /*
   verifyFormat("class A;\n"
                "namespace B\n"
                "  {\n"
                "class C;\n"
                "// Comment\n"
                "class D\n"
                "  {\n"
                "public:\n"
                "  D();\n"
                "  ~D() {}\n"
                "private:\n"
                "  enum E\n"
                "    {\n"
                "    F\n"
                "    }\n"
                "  };\n"
                "  } // namespace B\n",
                WhitesmithsBraceStyle);
   */
 
   verifyFormat("namespace a\n"
                "  {\n"
                "class A\n"
                "  {\n"
                "  void f()\n"
                "    {\n"
                "    if (true)\n"
                "      {\n"
                "      a();\n"
                "      b();\n"
                "      }\n"
verifyFormat("void f()\n"
                "    }\n"
                "  do\n"
                "    {\n"
                "    c();\n"
                "    } while (false)\n"
                "  }\n",
                WhitesmithsBraceStyle);
 
-  WhitesmithsBraceStyle.IndentCaseBlocks = true;
+  WhitesmithsBraceStyle.IndentCaseLabels = true;
   verifyFormat("void switchTest1(int a)\n"
                "  {\n"
                "  switch (a)\n"
                "    {\n"
                "    case 2:\n"
                "      {\n"
                "      }\n"
-               "    break;\n"
+               "      break;\n"
                "    }\n"
                "  }\n",
                WhitesmithsBraceStyle);
 
   verifyFormat("void switchTest2(int a)\n"
                "  {\n"
                "  switch (a)\n"
                "    {\n"
                "    case 0:\n"
-               "    break;\n"
+               "      break;\n"
                "    case 1:\n"
                "      {\n"
                "      break;\n"
                "      }\n"
                "    case 2:\n"
                "      {\n"
                "      }\n"
-               "    break;\n"
+               "      break;\n"
                "    default:\n"
-               "    break;\n"
+               "      break;\n"
                "    }\n"
                "  }\n",
                WhitesmithsBraceStyle);
 
   verifyFormat("void switchTest3(int a)\n"
                "  {\n"
                "  switch (a)\n"
                "    {\n"
                "    case 0:\n"
                "      {\n"
                "      foo(x);\n"
                "      }\n"
-               "    break;\n"
+               "      break;\n"
                "    default:\n"
                "      {\n"
                "      foo(1);\n"
                "      }\n"
-               "    break;\n"
+               "      break;\n"
                "    }\n"
                "  }\n",
                WhitesmithsBraceStyle);
 
-  WhitesmithsBraceStyle.IndentCaseBlocks = false;
+  WhitesmithsBraceStyle.IndentCaseLabels = false;
 
   verifyFormat("void switchTest4(int a)\n"
                "  {\n"
                "  switch (a)\n"
                "    {\n"
                "  case 2:\n"
                "    {\n"
                "    }\n"
verifyFormat("void switchTest6(int a)\n"
                "  default:\n"
                "    {\n"
                "    foo(1);\n"
                "    }\n"
                "    break;\n"
                "    }\n"
                "  }\n",
                WhitesmithsBraceStyle);
 
   verifyFormat("enum X\n"
                "  {\n"
                "  Y = 0, // testing\n"
                "  }\n",
                WhitesmithsBraceStyle);
 
   verifyFormat("enum X\n"
                "  {\n"