[clang-format][NFC] Code Tidies in UnwrappedLineFormatter

[HazardyKnusperkeks]

Summary

• Give I[1] and I[-1] a name:
  • Easier to understand
  • Easier to debug (since you don't go through operator[] everytime)
• TheLine->First != TheLine->Last follows since last is a l brace and first isn't.
• Factor the check for is(tok::l_brace) out.
• Drop else after return.

[curdeius]

LGTM. Don't forget to reformat please.

[owenpan]

How about naming I[-1] as well? And maybe I[i + 1] too?

[HazardyKnusperkeks]

Fixed formatting and added a name for I[-1].

I[i + 1] is used in the loop condition, which also checks if i + 1 is valid, so do not give a name here.

[owenpan]

LGTM.

[curdeius]

@HazardyKnusperkeks, it *seems* as if this commit (or one of others indicated on Changes tab of the link below) provoked failures in ASan. But it could be something else.
https://lab.llvm.org/buildbot/#/builders/5/builds/16734
Could you have a look?

[HazardyKnusperkeks]

In D115060#3219116, @curdeius wrote:

@HazardyKnusperkeks, it *seems* as if this commit (or one of others indicated on Changes tab of the link below) provoked failures in ASan. But it could be something else.
https://lab.llvm.org/buildbot/#/builders/5/builds/16734
Could you have a look?

Okay, I don't have access to ASAN or so, I'm using MinGW on Windows. But I suspect it is the Assignment of the PreviousLine since this is not existent every time. So I see the following solutions:

1. Only name NextLine, and use I[-1].
2. const auto HasPreviousLine = I != AnnotatedLines.begin(); const auto &PreviousLine = HasPreviousLine ? *I[-1] : *I; which is safe, since PreviousLine is only used if HasPreviousLine is true, but is a bit confusing. It would get an explaining comment.
3. Rearrange the statements so that we can have only one check if there is a previous line and define PreviousLine inside that if. It remains to be seen if that's NFC.

I would prefer option 3, but if that would change the behavior would go for option 2.

[HazardyKnusperkeks]

Option 3

[curdeius]

That's look good. And it's a nice clean up with all those repeated checks removed. Don't forget to reformat before landing.

[owenpan]

But I suspect it is the Assignment of the PreviousLine since this is not existent every time.

Yep!

So I see the following solutions:

1. Only name NextLine, and use I[-1].
2. const auto HasPreviousLine = I != AnnotatedLines.begin(); const auto &PreviousLine = HasPreviousLine ? *I[-1] : *I; which is safe, since PreviousLine is only used if HasPreviousLine is true, but is a bit confusing. It would get an explaining comment.
3. Rearrange the statements so that we can have only one check if there is a previous line and define PreviousLine inside that if. It remains to be seen if that's NFC.

I would prefer option 3, but if that would change the behavior would go for option 2.

There is option 4: change the type of PreviousLine to a pointer, and replace PreviousLine. with PreviousLine->. (You can rename PreviousLine if you think the naming is inconsistent with NextLine.) This would keep the patch being NFC.

[HazardyKnusperkeks]

In D115060#3224176, @owenpan wrote:

But I suspect it is the Assignment of the PreviousLine since this is not existent every time.

Yep!

So I see the following solutions:

1. Only name NextLine, and use I[-1].
2. const auto HasPreviousLine = I != AnnotatedLines.begin(); const auto &PreviousLine = HasPreviousLine ? *I[-1] : *I; which is safe, since PreviousLine is only used if HasPreviousLine is true, but is a bit confusing. It would get an explaining comment.
3. Rearrange the statements so that we can have only one check if there is a previous line and define PreviousLine inside that if. It remains to be seen if that's NFC.

I would prefer option 3, but if that would change the behavior would go for option 2.

There is option 4: change the type of PreviousLine to a pointer, and replace PreviousLine. with PreviousLine->. (You can rename PreviousLine if you think the naming is inconsistent with NextLine.) This would keep the patch being NFC.

I'm open for that, but I think the current diff (plus corrected formatting) is better. And at least according to our tests it is NFC.

[owenpan]

Moving the handling of empty record blocks might be non-NFC? Maybe option 4 for now and the refactoring in another patch?

clang/lib/Format/UnwrappedLineFormatter.cpp

360

Is TheLine->First != TheLine->Last redundant?

381

I don't think you can drop else here.

388

Ditto.

406–431

Handling empty record blocks here instead of earlier may have unknown side effects?

[HazardyKnusperkeks]

In D115060#3227120, @owenpan wrote:

Maybe option 4 for now

So I can get it through.

and the refactoring in another patch?

Probably won't do it.

clang/lib/Format/UnwrappedLineFormatter.cpp

388

Same.

406–431

Of course it may have side effects.

[HazardyKnusperkeks]

Updated, now using a pointer (potential null) for PreviousLine.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

360

This seems to be an error on my side.

[owenpan]

clang/lib/Format/UnwrappedLineFormatter.cpp

215

I would move this line to just before handling empty record blocks below.

266–267

I'd either leave this lambda alone or further simplify it as suggested here.

316

Drop the I if you decide to further simplify the lambda as suggested above.

381

I don't think you can drop else here.

I was wrong. For some reason, I didn't see the return statement.

388

I missed the return here as well.

403–414

If you want, you can factor out PreviousLine && TheLine->First->is(tok::l_brace) and even combine the two if statements as they both return 0.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

215

I'd rather keep the definitions close together.

266–267

I'm no fan of capturing everything, but some things I've applied.

403–414

Kept the two cases apart for their comments.

[curdeius]

LGTM.

[owenpan]

clang/lib/Format/UnwrappedLineFormatter.cpp

215

If it were just a simple initialization, it wouldn't matter much. However, it would be a bit wasteful as PreviousLine always gets computed here even if the function may return before the pointer would get chance to be used. If you really want to keep all related definitions together, wouldn't you want to move lines 214-215 up to right after line 211?

266–267

[&] doesn't capture everything. It only captures the variables that are used in the lambda body. That being said, I'm okay with explicitly capturing each one.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

215

In a release build I'm betting that the compiler is smart enough to never calculate PreviousLine and that performance doesn't really matter was shown in D116318.

But yeah moving it up to TheLine is consistent and will do.

[owenpan]

clang/lib/Format/UnwrappedLineFormatter.cpp

215

In a release build I'm betting that the compiler is smart enough to never calculate PreviousLine

No compiler will (or should) skip generating code that calculates PreviousLine, which may or may not get used at runtime. I'm not aware of any compiler that is smart enough to move the initialization code to just before where the variable is first used.

To illustrate the difference, I compiled the code below with clang++ -Wall -std=c++11 -O3 -S foo.cpp:

#include <vector>

using namespace std;
using Type = vector<int *>;

int f(const Type &V, Type::const_iterator I) {
  const auto *P = I != V.begin() ? I[-1] : nullptr;
  if (I == V.end())
    return 0;
  return *P;
}

The generated code in foo.s includes the following:

	.cfi_startproc
; %bb.0:
	ldr	x8, [x0]
	cmp	x8, x1
	b.eq	LBB0_3
; %bb.1:
	ldur	x8, [x1, #-8]
	ldr	x9, [x0, #8]
	cmp	x9, x1
	b.eq	LBB0_4
LBB0_2:
	ldr	w0, [x8]
	ret
LBB0_3:
	mov	x8, #0
	ldr	x9, [x0, #8]
	cmp	x9, x1
	b.ne	LBB0_2
LBB0_4:
	mov	w0, #0
	ret
	.cfi_endproc

As you can see, the initialization code was neither optimized away nor moved.

When I changed the function body to:

if (I == V.end())
  return 0;
const auto *P = I != V.begin() ? I[-1] : nullptr;
return *P;

The generated code became much simpler:

	.cfi_startproc
; %bb.0:
	ldr	x8, [x0, #8]
	cmp	x8, x1
	b.eq	LBB0_2
; %bb.1:
	ldur	x8, [x1, #-8]
	ldr	w0, [x8]
	ret
LBB0_2:
	mov	w0, #0
	ret
	.cfi_endproc

[owenpan]

clang/lib/Format/UnwrappedLineFormatter.cpp

215

and that performance doesn't really matter was shown in D116318.

It wasn't quite the same there as we must pay either in the caller or in the callee. That being said, I agree that the performance hit here would be negligible.

[MyDeveloperDay]

LGTM

clang/lib/Format/UnwrappedLineFormatter.cpp

tryFitMultipleLinesInOne(LevelIndentTracker &IndentTracker,
 
     // Can't join the last line with anything.
     if (I + 1 == E)
       return 0;
     // We can never merge stuff if there are trailing line comments.
     const AnnotatedLine *TheLine = *I;
     if (TheLine->Last->is(TT_LineComment))
       return 0;
-    if (I[1]->Type == LT_Invalid || I[1]->First->MustBreakBefore)
+    const auto &NextLine = *I[1];
+    const auto *PreviousLine = I != AnnotatedLines.begin() ? I[-1] : nullptr;
+    if (NextLine.Type == LT_Invalid || NextLine.First->MustBreakBefore)
       return 0;
     if (TheLine->InPPDirective &&
-        (!I[1]->InPPDirective || I[1]->First->HasUnescapedNewline))
+        (!NextLine.InPPDirective || NextLine.First->HasUnescapedNewline))
       return 0;
 
     if (Style.ColumnLimit > 0 && Indent > Style.ColumnLimit)
       return 0;
 
     unsigned Limit =
         Style.ColumnLimit == 0 ? UINT_MAX : Style.ColumnLimit - Indent;
     // If we already exceed the column limit, we set 'Limit' to 0. The different
     // tryMerge..() functions can then decide whether to still do merging.
     Limit = TheLine->Last->TotalLength > Limit
                 ? 0
                 : Limit - TheLine->Last->TotalLength;
 
     if (TheLine->Last->is(TT_FunctionLBrace) &&
         TheLine->First == TheLine->Last &&
         !Style.BraceWrapping.SplitEmptyFunction &&
-        I[1]->First->is(tok::r_brace))
+        NextLine.First->is(tok::r_brace))
       return tryMergeSimpleBlock(I, E, Limit);
 
-    // Handle empty record blocks where the brace has already been wrapped
-    if (TheLine->Last->is(tok::l_brace) && TheLine->First == TheLine->Last &&
-        I != AnnotatedLines.begin()) {
-      bool EmptyBlock = I[1]->First->is(tok::r_brace);
+    // Handle empty record blocks where the brace has already been wrapped.
+    if (PreviousLine && TheLine->Last->is(tok::l_brace) &&
+        TheLine->First == TheLine->Last) {
+      bool EmptyBlock = NextLine.First->is(tok::r_brace);
 
-      const FormatToken *Tok = I[-1]->First;
+      const FormatToken *Tok = PreviousLine->First;
       if (Tok && Tok->is(tok::comment))
         Tok = Tok->getNextNonComment();
 
       if (Tok && Tok->getNamespaceToken())
         return !Style.BraceWrapping.SplitEmptyNamespace && EmptyBlock
                    ? tryMergeSimpleBlock(I, E, Limit)
                    : 0;
 
       if (Tok && Tok->is(tok::kw_typedef))
         Tok = Tok->getNextNonComment();
       if (Tok && Tok->isOneOf(tok::kw_class, tok::kw_struct, tok::kw_union,
                               tok::kw_extern, Keywords.kw_interface))
         return !Style.BraceWrapping.SplitEmptyRecord && EmptyBlock
                    ? tryMergeSimpleBlock(I, E, Limit)
                    : 0;
 
       if (Tok && Tok->is(tok::kw_template) &&
           Style.BraceWrapping.SplitEmptyRecord && EmptyBlock) {
         return 0;
       }
     }
 
-    auto ShouldMergeShortFunctions =
-        [this, B = AnnotatedLines.begin()](
-            SmallVectorImpl<AnnotatedLine *>::const_iterator I) {
-          if (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All)
-            return true;
-          if (Style.AllowShortFunctionsOnASingleLine >=
-                  FormatStyle::SFS_Empty &&
-              I[1]->First->is(tok::r_brace))
-            return true;
+    auto ShouldMergeShortFunctions = [this, &I, &NextLine, PreviousLine,
+                                      TheLine]() {
+      if (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
+          NextLine.First->is(tok::r_brace))
 
-          if (Style.AllowShortFunctionsOnASingleLine &
-              FormatStyle::SFS_InlineOnly) {
-            // Just checking TheLine->Level != 0 is not enough, because it
-            // provokes treating functions inside indented namespaces as short.
-            if ((*I)->Level != 0) {
-              if (I == B)
-                return false;
+        if (TheLine->Level != 0) {
+          if (!PreviousLine)
 
-              // TODO: Use IndentTracker to avoid loop?
-              // Find the last line with lower level.
-              auto J = I - 1;
-              for (; J != B; --J)
-                if ((*J)->Level < (*I)->Level)
-                  break;
+          for (; J != AnnotatedLines.begin(); --J)
+            if ((*J)->Level < TheLine->Level)
 
-              // Check if the found line starts a record.
-              auto *RecordTok = (*J)->First;
-              while (RecordTok) {
-                // TODO: Refactor to isRecord(RecordTok).
-                if (RecordTok->isOneOf(tok::kw_class, tok::kw_struct))
-                  return true;
-                if (Style.isCpp() && RecordTok->is(tok::kw_union))
-                  return true;
-                if (Style.isCSharp() && RecordTok->is(Keywords.kw_interface))
-                  return true;
-                if (Style.Language == FormatStyle::LK_Java &&
-                    RecordTok->is(tok::kw_enum))
-                  return true;
-                if (Style.isJavaScript() && RecordTok->is(Keywords.kw_function))
-                  return true;
 
-                RecordTok = RecordTok->Next;
-              }
 
-              return false;
-            }
-          }
 
-          return false;
-        };
 
-    bool MergeShortFunctions = ShouldMergeShortFunctions(I);
+    bool MergeShortFunctions = ShouldMergeShortFunctions();
 
     if (Style.CompactNamespaces) {
       if (auto nsToken = TheLine->First->getNamespaceToken()) {
         int i = 0;
         unsigned closingLine = TheLine->MatchingClosingBlockLineIndex - 1;
         for (; I + 1 + i != E &&
                nsToken->TokenText == getNamespaceTokenText(I[i + 1]) &&
                closingLine == I[i + 1]->MatchingClosingBlockLineIndex &&
                I[i + 1]->Last->TotalLength < Limit;
              i++, closingLine--) {
-          // No extra indent for compacted namespaces
+          // No extra indent for compacted namespaces.
           IndentTracker.skipLine(*I[i + 1]);
 
           Limit -= I[i + 1]->Last->TotalLength;
         }
         return i;
       }
 
       if (auto nsToken = getMatchingNamespaceToken(TheLine, AnnotatedLines)) {
         int i = 0;
         unsigned openingLine = TheLine->MatchingOpeningBlockLineIndex - 1;
         for (; I + 1 + i != E &&
                nsToken->TokenText ==
                    getMatchingNamespaceTokenText(I[i + 1], AnnotatedLines) &&
                openingLine == I[i + 1]->MatchingOpeningBlockLineIndex;
              i++, openingLine--) {
-          // No space between consecutive braces
+          // No space between consecutive braces.
           I[i + 1]->First->SpacesRequiredBefore = !I[i]->Last->is(tok::r_brace);
 
-          // Indent like the outer-most namespace
+          // Indent like the outer-most namespace.
           IndentTracker.nextLine(*I[i + 1]);
         }
         return i;
       }
     }
 
-    // Try to merge a function block with left brace unwrapped
+    // Try to merge a function block with left brace unwrapped.
     if (TheLine->Last->is(TT_FunctionLBrace) &&
         TheLine->First != TheLine->Last) {
       return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
     }
-    // Try to merge a control statement block with left brace unwrapped
+    // Try to merge a control statement block with left brace unwrapped.
     if (TheLine->Last->is(tok::l_brace) && TheLine->First != TheLine->Last &&
         TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
       return Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never
                  ? tryMergeSimpleBlock(I, E, Limit)
                  : 0;
     }
-    // Try to merge a control statement block with left brace wrapped
-    if (I[1]->First->is(tok::l_brace) &&
-        (TheLine->First->isOneOf(tok::kw_if, tok::kw_else, tok::kw_while,
+    // Try to merge a control statement block with left brace wrapped.
+    if (NextLine.First->is(tok::l_brace)) {
+      if ((TheLine->First->isOneOf(tok::kw_if, tok::kw_else, tok::kw_while,
                                    tok::kw_for, tok::kw_switch, tok::kw_try,
                                    tok::kw_do, TT_ForEachMacro) ||
            (TheLine->First->is(tok::r_brace) && TheLine->First->Next &&
             TheLine->First->Next->isOneOf(tok::kw_else, tok::kw_catch))) &&
           Style.BraceWrapping.AfterControlStatement ==
               FormatStyle::BWACS_MultiLine) {
-      // If possible, merge the next line's wrapped left brace with the current
-      // line. Otherwise, leave it on the next line, as this is a multi-line
-      // control statement.
+        // If possible, merge the next line's wrapped left brace with the
+        // current line. Otherwise, leave it on the next line, as this is a
+        // multi-line control statement.
         return (Style.ColumnLimit == 0 ||
                 TheLine->Last->TotalLength <= Style.ColumnLimit)
                    ? 1
                    : 0;
-    } else if (I[1]->First->is(tok::l_brace) &&
-               TheLine->First->isOneOf(tok::kw_if, tok::kw_else, tok::kw_while,
-                                       tok::kw_for)) {
+      }
+      if (TheLine->First->isOneOf(tok::kw_if, tok::kw_else, tok::kw_while,
         return (Style.BraceWrapping.AfterControlStatement ==
                 FormatStyle::BWACS_Always)
                    ? tryMergeSimpleBlock(I, E, Limit)
                    : 0;
-    } else if (I[1]->First->is(tok::l_brace) &&
-               TheLine->First->isOneOf(tok::kw_else, tok::kw_catch) &&
-               Style.BraceWrapping.AfterControlStatement ==
-                   FormatStyle::BWACS_MultiLine) {
+      }
+      if (TheLine->First->isOneOf(tok::kw_else, tok::kw_catch) &&
         // This case if different from the upper BWACS_MultiLine processing
         // in that a preceding r_brace is not on the same line as else/catch
         // most likely because of BeforeElse/BeforeCatch set to true.
         // If the line length doesn't fit ColumnLimit, leave l_brace on the
         // next line to respect the BWACS_MultiLine.
         return (Style.ColumnLimit == 0 ||
                 TheLine->Last->TotalLength <= Style.ColumnLimit)
                    ? 1
                    : 0;
       }
-    // Don't merge block with left brace wrapped after ObjC special blocks
-    if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
-        I[-1]->First->is(tok::at) && I[-1]->First->Next) {
-      tok::ObjCKeywordKind kwId = I[-1]->First->Next->Tok.getObjCKeywordID();
-      if (kwId == clang::tok::objc_autoreleasepool ||
-          kwId == clang::tok::objc_synchronized)
+    }
+    if (PreviousLine && TheLine->First->is(tok::l_brace)) {
+      switch (PreviousLine->First->Tok.getKind()) {
+      case tok::at:
+        // Don't merge block with left brace wrapped after ObjC special blocks.
+        if (PreviousLine->First->Next) {
+          tok::ObjCKeywordKind kwId =
+              PreviousLine->First->Next->Tok.getObjCKeywordID();
+          if (kwId == tok::objc_autoreleasepool ||
+              kwId == tok::objc_synchronized)
             return 0;
         }
-    // Don't merge block with left brace wrapped after case labels
-    if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
-        I[-1]->First->isOneOf(tok::kw_case, tok::kw_default))
+        break;
+
+      case tok::kw_case:
+      case tok::kw_default:
+        // Don't merge block with left brace wrapped after case labels.
         return 0;
 
+      default:
+        break;
+      }
+    }
+
     // Don't merge an empty template class or struct if SplitEmptyRecords
     // is defined.
-    if (Style.BraceWrapping.SplitEmptyRecord &&
-        TheLine->Last->is(tok::l_brace) && I != AnnotatedLines.begin() &&
-        I[-1]->Last) {
-      const FormatToken *Previous = I[-1]->Last;
+    if (PreviousLine && Style.BraceWrapping.SplitEmptyRecord &&
+        TheLine->Last->is(tok::l_brace) && PreviousLine->Last) {
+      const FormatToken *Previous = PreviousLine->Last;
       if (Previous) {
         if (Previous->is(tok::comment))
           Previous = Previous->getPreviousNonComment();
         if (Previous) {
-          if (Previous->is(tok::greater) && !I[-1]->InPPDirective)
+          if (Previous->is(tok::greater) && !PreviousLine->InPPDirective)
             return 0;
           if (Previous->is(tok::identifier)) {
             const FormatToken *PreviousPrevious =
                 Previous->getPreviousNonComment();
             if (PreviousPrevious &&
                 PreviousPrevious->isOneOf(tok::kw_class, tok::kw_struct))
               return 0;
           }
         }
       }
     }
 
-    // Try to merge a block with left brace unwrapped that wasn't yet covered
+    // Try to merge a block with left brace unwrapped that wasn't yet covered.
     if (TheLine->Last->is(tok::l_brace)) {
       const FormatToken *Tok = TheLine->First;
       bool ShouldMerge = false;
       if (Tok->is(tok::kw_typedef)) {
         Tok = Tok->getNextNonComment();
         assert(Tok);
       }
       if (Tok->isOneOf(tok::kw_class, tok::kw_struct)) {
         ShouldMerge = !Style.BraceWrapping.AfterClass ||
-                      (I[1]->First->is(tok::r_brace) &&
+                      (NextLine.First->is(tok::r_brace) &&
                        !Style.BraceWrapping.SplitEmptyRecord);
       } else if (Tok->is(tok::kw_enum)) {
         ShouldMerge = Style.AllowShortEnumsOnASingleLine;
       } else {
         ShouldMerge = !Style.BraceWrapping.AfterFunction ||
-                      (I[1]->First->is(tok::r_brace) &&
+                      (NextLine.First->is(tok::r_brace) &&
                        !Style.BraceWrapping.SplitEmptyFunction);
       }
       return ShouldMerge ? tryMergeSimpleBlock(I, E, Limit) : 0;
     }
-    // Try to merge a function block with left brace wrapped
-    if (I[1]->First->is(TT_FunctionLBrace) &&
+    // Try to merge a function block with left brace wrapped.
+    if (NextLine.First->is(TT_FunctionLBrace) &&
         Style.BraceWrapping.AfterFunction) {
-      if (I[1]->Last->is(TT_LineComment))
+      if (NextLine.Last->is(TT_LineComment))
         return 0;
 
       // Check for Limit <= 2 to account for the " {".
       if (Limit <= 2 || (Style.ColumnLimit == 0 && containsMustBreak(TheLine)))
         return 0;
       Limit -= 2;
 
       unsigned MergedLines = 0;
       if (MergeShortFunctions ||
           (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
-           I[1]->First == I[1]->Last && I + 2 != E &&
+           NextLine.First == NextLine.Last && I + 2 != E &&
            I[2]->First->is(tok::r_brace))) {
         MergedLines = tryMergeSimpleBlock(I + 1, E, Limit);
-        // If we managed to merge the block, count the function header, which is
-        // on a separate line.
+        // If we managed to merge the block, count the function header, which
+        // is on a separate line.
         if (MergedLines > 0)
           ++MergedLines;
       }
       return MergedLines;
     }
     auto IsElseLine = [&TheLine]() -> bool {
       const FormatToken *First = TheLine->First;
       if (First->is(tok::kw_else))