[PATCH] Refactor clang-format's formatter.

[Daniel Jasper]

I like it.


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:28
@@ -27,1 +27,3 @@
 
+/// \brief Tracks the indent level for an \c AnnotatedLine across levels.
+struct LevelIndentTracker {
----------------
I think "Tracks the indent level of \c AnnotatedLines across levels" is slightly clearer as this tracks the indent for multiple lines.

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:29
@@ +28,3 @@
+/// \brief Tracks the indent level for an \c AnnotatedLine across levels.
+struct LevelIndentTracker {
+  LevelIndentTracker(const FormatStyle &Style,
----------------
I think this should be a class, not a struct.

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:34
@@ +33,3 @@
+      : Style(Style), Keywords(Keywords) {
+    for (unsigned i = 0, e = StartLevel; i != e; ++i)
+      IndentForLevel.push_back(Style.IndentWidth * i + AdditionalIndent);
----------------
Just use:

  for (unsigned i = 0; i != StartLevel; ++i) ...

StartLevel already is a POD type (unsigned even) and we don't get anything.

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:39-43
@@ +38,7 @@
+  /// \brief Returns the indent for the current line.
+  unsigned getIndent() const { return Indent; }
+
+  /// \brief Update the indent state given that \p Line is going to be formatted
+  /// next.
+  void updateIndentForLine(const AnnotatedLine &Line) {
+    Offset = getIndentOffset(*Line.First);
----------------
Ignore.. Phab doesn't let me delete this comment (Eclipse-like behavior).

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:62
@@ +61,3 @@
+  void updateLevelIndentFromUnmodifiedLine(const AnnotatedLine &Line) {
+    unsigned LevelIndent = Line.First->OriginalColumn;
+    if (static_cast<int>(LevelIndent) - Offset >= 0)
----------------
Isn't this missing an

  Offset = getIndentOffset(*Line.First);

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:106-110
@@ +105,7 @@
+
+  /// \brief Offset of the current line relative to the indent level.
+  ///
+  /// For example, the 'public' keywords is often indented with a negative
+  /// offset.
+  int Offset = 0;
+
----------------
As obvious from my comment above where I probably show that I am not understanding something, it is unclear how this needs to be a class variable instead of a variable calculated per line. Please comment.

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:132-144
@@ +131,15 @@
+        tryFitMultipleLinesInOne(IndentTracker.getIndent(), Next, End);
+    if (MergedLines > 0 && Style.ColumnLimit == 0) {
+      // Disallow line merging if there is a break at the start of one of the
+      // input lines.
+      for (unsigned i = 0; i < MergedLines; ++i) {
+        if (Next[i + 1]->First->NewlinesBefore > 0)
+          MergedLines = 0;
+      }
+    }
+    if (!DryRun) {
+      for (unsigned i = 0; i < MergedLines; ++i) {
+        join(*Next[i], *Next[i + 1]);
+      }
+    }
+    Next = Next + MergedLines + 1;
----------------
Remove all the braces.

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:440-441
@@ -308,2 +439,4 @@
   const AdditionalKeywords &Keywords;
+  const SmallVectorImpl<AnnotatedLine*>::const_iterator End;
+  SmallVectorImpl<AnnotatedLine*>::const_iterator Next;
 };
----------------
Can you change the order? My OCD finds it weird to have Next after End ;-).

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:815-819
@@ +814,7 @@
+    bool Format = TheLine.Type != LT_Invalid && ShouldFormat;
+    // If we format a line, each AnnotatedLine will start a new line (otherwise
+    // we merge them into the same AnnotatedLine).
+    // If we do not format a line, this does not hold true - in that case an
+    // arbitrary number of annotated lines can be in one line.
+    bool AnnotatedLineStartsNewLine =
+        TheLine.First->NewlinesBefore > 0 || TheLine.First->IsFirst;
----------------
Both, the comment and the variable name are a bit confusing. Lets discuss this offline.

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:829
@@ +828,3 @@
+    if (!Format && ShouldFormat && IncompleteFormat) {
+      assert(TheLine.Type == LT_Invalid);
+      *IncompleteFormat = true;
----------------
This assert seems a bit odd ..

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:833
@@ -710,20 +832,3 @@
 
-    bool FixIndentation =
-        FixBadIndentation && (LevelIndent != FirstTok->OriginalColumn);
-    bool ShouldFormat = TheLine.Affected || FixIndentation;
-    if (TheLine.First->is(tok::eof)) {
-      if (PreviousLine && PreviousLine->Affected && !DryRun) {
-        // Remove the file's trailing whitespace.
-        unsigned Newlines = std::min(FirstTok->NewlinesBefore, 1u);
-        Whitespaces->replaceWhitespace(*TheLine.First, Newlines,
-                                       /*IndentLevel=*/0, /*Spaces=*/0,
-                                       /*TargetColumn=*/0);
-      }
-    } else if (TheLine.Type != LT_Invalid && ShouldFormat) {
-      if (FirstTok->WhitespaceRange.isValid()) {
-        if (!DryRun)
-          formatFirstToken(*TheLine.First, PreviousLine, TheLine.Level, Indent,
-                           TheLine.InPPDirective);
-      } else {
-        Indent = LevelIndent = FirstTok->OriginalColumn;
-      }
+    if (Format) {
+      if (!DryRun)
----------------
The actions for "Format" and "!Format" seem pretty interleaved here. Couldn't we group them more?

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:843
@@ -738,7 +842,3 @@
 
-      if (TheLine.Last->TotalLength + Indent <= ColumnLimit ||
-          TheLine.Type == LT_ImportStatement) {
-        Penalty += NoLineBreakFormatter(Indenter, Whitespaces, Style, this)
-                       .formatLine(TheLine, Indent, DryRun);
-      } else if (Style.ColumnLimit == 0) {
+      if (Style.ColumnLimit == 0) {
         NoColumnLimitLineFormatter(Indenter, Whitespaces, Style, this)
----------------
No braces.. Here and below.

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:895
@@ +894,3 @@
+    Whitespaces->replaceWhitespace(RootToken, Newlines, /*IndentLevel=*/0,
+                                   /*Spaces=*/0,
+                                   /*TargetColumn=*/0);
----------------
Merge this line with the next (there already is a clang-format bug for that).

================
Comment at: lib/Format/UnwrappedLineFormatter.h:53
@@ -68,15 +52,3 @@
 
-  /// \brief Get the indent of \p Level from \p IndentForLevel.
-  ///
-  /// \p IndentForLevel must contain the indent for the level \c l
-  /// at \p IndentForLevel[l], or a value < 0 if the indent for
-  /// that level is unknown.
-  unsigned getIndent(ArrayRef<int> IndentForLevel, unsigned Level);
-
-  void join(AnnotatedLine &A, const AnnotatedLine &B);
-
-  unsigned getColumnLimit(bool InPPDirective) const {
-    // In preprocessor directives reserve two chars for trailing " \"
-    return Style.ColumnLimit - (InPPDirective ? 2 : 0);
-  }
+  unsigned getColumnLimit(bool InPPDirective, const AnnotatedLine *NextLine) const;
 
----------------
If the implementation isn't in the header anymore, this might deserve a small comment.

http://reviews.llvm.org/D9662

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/