[PATCH] Fix for corner cases in code handling leading "* " decorations in block comments

Alexander Kornienko alexfh at google.com
Mon Jul 8 06:56:06 PDT 2013


Hi klimek,

Fixes problems that lead to incorrect formatting of these and similar snippets:
/*
 **
 */

/*
 **/

/*
 * */

/*
 *test
 */

Clang-format used to think that all the cases above use "* " decoration, and
failed to calculate insertion position properly. It also used to remove leading 
"* " in the last line.

http://llvm-reviews.chandlerc.com/D1113

Files:
  lib/Format/BreakableToken.cpp
  lib/Format/BreakableToken.h
  unittests/Format/FormatTest.cpp

Index: lib/Format/BreakableToken.cpp
===================================================================
--- lib/Format/BreakableToken.cpp
+++ lib/Format/BreakableToken.cpp
@@ -225,50 +225,56 @@
   TokenText.substr(2, TokenText.size() - 4).split(Lines, "\n");
 
   int IndentDelta = StartColumn - OriginalStartColumn;
-  bool NeedsStar = true;
   LeadingWhitespace.resize(Lines.size());
   StartOfLineColumn.resize(Lines.size());
+  StartOfLineColumn[0] = StartColumn + 2;
+  for (size_t i = 1; i < Lines.size(); ++i)
+    adjustWhitespace(Style, i, IndentDelta);
+
+  Decoration = "* ";
   if (Lines.size() == 1 && !FirstInLine) {
     // Comments for which FirstInLine is false can start on arbitrary column,
     // and available horizontal space can be too small to align consecutive
     // lines with the first one.
     // FIXME: We could, probably, align them to current indentation level, but
     // now we just wrap them without stars.
-    NeedsStar = false;
+    Decoration = "";
   }
-  StartOfLineColumn[0] = StartColumn + 2;
-  for (size_t i = 1; i < Lines.size(); ++i) {
-    adjustWhitespace(Style, i, IndentDelta);
-    if (Lines[i].empty())
-      // If the last line is empty, the closing "*/" will have a star.
-      NeedsStar = NeedsStar && i + 1 == Lines.size();
-    else
-      NeedsStar = NeedsStar && Lines[i][0] == '*';
+  for (size_t i = 1, e = Lines.size(); i < e && !Decoration.empty(); ++i) {
+    // If the last line is empty, the closing "*/" will have a star.
+    if (i == e - 1 && Lines[i].empty())
+      continue;
+    while (!Lines[i].startswith(Decoration))
+      Decoration = Decoration.substr(0, Decoration.size() - 1);
   }
-  Decoration = NeedsStar ? "* " : "";
+
+  LastLineNeedsDecoration = true;
   IndentAtLineBreak = StartOfLineColumn[0] + 1;
   for (size_t i = 1; i < Lines.size(); ++i) {
     if (Lines[i].empty()) {
-      if (!NeedsStar && i + 1 != Lines.size())
-        // For all but the last line (which always ends in */), set the
-        // start column to 0 if they're empty, so we do not insert
-        // trailing whitespace anywhere.
+      if (i + 1 == Lines.size()) {
+        // Empty last line means that we already have a star as a part of the
+        // trailing */. We also need to preserve whitespace, so that */ is
+        // correctly indented.
+        LastLineNeedsDecoration = false;
+      } else if (Decoration.empty()) {
+        // For all other lines, set the start column to 0 if they're empty, so
+        // we do not insert trailing whitespace anywhere.
         StartOfLineColumn[i] = 0;
+      }
       continue;
     }
-    if (NeedsStar) {
-      // The first line already excludes the star.
-      // For all other lines, adjust the line to exclude the star and
-      // (optionally) the first whitespace.
-      int Offset = Lines[i].startswith("* ") ? 2 : 1;
-      StartOfLineColumn[i] += Offset;
-      Lines[i] = Lines[i].substr(Offset);
-      LeadingWhitespace[i] += Offset;
-    }
+    // The first line already excludes the star.
+    // For all other lines, adjust the line to exclude the star and
+    // (optionally) the first whitespace.
+    StartOfLineColumn[i] += Decoration.size();
+    Lines[i] = Lines[i].substr(Decoration.size());
+    LeadingWhitespace[i] += Decoration.size();
     IndentAtLineBreak = std::min<int>(IndentAtLineBreak, StartOfLineColumn[i]);
   }
   IndentAtLineBreak = std::max<unsigned>(IndentAtLineBreak, Decoration.size());
   DEBUG({
+    llvm::dbgs() << "IndentAtLineBreak " << IndentAtLineBreak << "\n";
     for (size_t i = 0; i < Lines.size(); ++i) {
       llvm::dbgs() << i << " |" << Lines[i] << "| " << LeadingWhitespace[i]
                    << "\n";
@@ -365,17 +371,19 @@
   StringRef Prefix = Decoration;
   if (Lines[LineIndex].empty()) {
     if (LineIndex + 1 == Lines.size()) {
-      // If the last line is empty, we don't need a prefix, as the */ will line
-      // up with the decoration (if it exists).
-      Prefix = "";
+      if (!LastLineNeedsDecoration) {
+        // If the last line is empty, we don't need a prefix, as the */ will
+        // line up with the decoration (if it exists).
+        Prefix = "";
+      }
     } else if (!Decoration.empty()) {
       // For other empty lines, if we do have a decoration, adapt it to not
       // contain a trailing whitespace.
       Prefix = Prefix.substr(0, 1);
     }
   } else {
     if (StartOfLineColumn[LineIndex] == 1) {
-      // This lines starts immediately after the decorating *.
+      // This line starts immediately after the decorating *.
       Prefix = Prefix.substr(0, 1);
     }
   }
Index: lib/Format/BreakableToken.h
===================================================================
--- lib/Format/BreakableToken.h
+++ lib/Format/BreakableToken.h
@@ -208,6 +208,10 @@
   // instead.
   unsigned IndentAtLineBreak;
 
+  // This is to distinguish between the case when the last line was empty and
+  // the case when it started with a decoration ("*" or "* ").
+  bool LastLineNeedsDecoration;
+
   // Either "* " if all lines begin with a "*", or empty.
   StringRef Decoration;
 };
Index: unittests/Format/FormatTest.cpp
===================================================================
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3967,7 +3967,7 @@
   EXPECT_EQ("/*\n"
             "*\n"
             " * aaaaaa\n"
-            "* aaaaaa\n"
+            "*aaaaaa\n"
             "*/",
             format("/*\n"
                    "*\n"
@@ -3977,7 +3977,7 @@
   EXPECT_EQ("/*\n"
             "**\n"
             "* aaaaaa\n"
-            "* aaaaaa\n"
+            "*aaaaaa\n"
             "*/",
             format("/*\n"
                    "**\n"
@@ -4017,6 +4017,33 @@
              "int    cccccccccccccccccccccccccccccc;  /* comment */\n"));
 
   verifyFormat("void f(int * /* unused */) {}");
+
+  EXPECT_EQ("/*\n"
+            " **\n"
+            " */",
+            format("/*\n"
+                   " **\n"
+                   " */"));
+  EXPECT_EQ("/*\n"
+            " *q\n"
+            " */",
+            format("/*\n"
+                   " *q\n"
+                   " */"));
+  EXPECT_EQ("/*\n"
+            " * q\n"
+            " */",
+            format("/*\n"
+                   " * q\n"
+                   " */"));
+  EXPECT_EQ("/*\n"
+            " **/",
+            format("/*\n"
+                   " **/"));
+  EXPECT_EQ("/*\n"
+            " ***/",
+            format("/*\n"
+                   " ***/"));
 }
 
 TEST_F(FormatTest, BlockCommentsInMacros) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D1113.1.patch
Type: text/x-patch
Size: 6566 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130708/294c6bfd/attachment.bin>


More information about the cfe-commits mailing list