clang-format: fix a crash in comment wraps.

[mprobst]

Summary

Previously, clang-format would crash if it tried to wrap an overlong
single line comment, because two parts of the code inserted a break in
the same location.

/** heregoesalongcommentwithnospace */

This wasn't previously noticed as it could only trigger for an overlong
single line comment that did have no breaking opportunities except for a
whitespace at the very beginning.

This also introduces a check for JavaScript to not ever wrap a comment
before an opening curly brace:

/** @mods {donotbreakbeforethecurly} */

This is because some machinery parsing these tags sometimes supports
breaks before a possible {, but in some other cases does not.
Previously clang-format was careful never to wrap a line with certain
tags on it. The better solution is to specifically disable wrapping
before the problematic token: this allows wrapping and aligning comments
but still avoids the problem.

[krasimir]

Great! Thank you!

cfe/trunk/lib/Format/BreakableToken.cpp

static StringRef getLineCommentIndentPrefix(StringRef Comment,
   }
   return LongestPrefix;
 }
 
 static BreakableToken::Split getCommentSplit(StringRef Text,
                                              unsigned ContentStartColumn,
                                              unsigned ColumnLimit,
                                              unsigned TabWidth,
-                                             encoding::Encoding Encoding) {
-  LLVM_DEBUG(llvm::dbgs() << "Comment split: \"" << Text << ", " << ColumnLimit
-                          << "\", Content start: " << ContentStartColumn
-                          << "\n");
+                                             encoding::Encoding Encoding,
+                                             const FormatStyle &Style) {
+  LLVM_DEBUG(llvm::dbgs() << "Comment split: \"" << Text
+                          << "\", Column limit: " << ColumnLimit
+                          << ", Content start: " << ContentStartColumn << "\n");
   if (ColumnLimit <= ContentStartColumn + 1)
     return BreakableToken::Split(StringRef::npos, 0);
 
   unsigned MaxSplit = ColumnLimit - ContentStartColumn + 1;
   unsigned MaxSplitBytes = 0;
 
   for (unsigned NumChars = 0;
        NumChars < MaxSplit && MaxSplitBytes < Text.size();) {
     unsigned BytesInChar =
         encoding::getCodePointNumBytes(Text[MaxSplitBytes], Encoding);
     NumChars +=
         encoding::columnWidthWithTabs(Text.substr(MaxSplitBytes, BytesInChar),
                                       ContentStartColumn, TabWidth, Encoding);
     MaxSplitBytes += BytesInChar;
   }
 
   StringRef::size_type SpaceOffset = Text.find_last_of(Blanks, MaxSplitBytes);
 
   // Do not split before a number followed by a dot: this would be interpreted
   // as a numbered list, which would prevent re-flowing in subsequent passes.
   static auto *const kNumberedListRegexp = new llvm::Regex("^[1-9][0-9]?\\.");
   if (SpaceOffset != StringRef::npos &&
       kNumberedListRegexp->match(Text.substr(SpaceOffset).ltrim(Blanks)))
     SpaceOffset = Text.find_last_of(Blanks, SpaceOffset);
+  // In JavaScript, some @tags can be followed by {, and machinery that parses
+  // these comments will fail to understand the comment if followed by a line
+  // break. So avoid ever breaking before a {.
+  if (Style.Language == FormatStyle::LK_JavaScript &&
+      SpaceOffset != StringRef::npos && SpaceOffset + 1 < Text.size() &&
+      Text[SpaceOffset + 1] == '{')
+    SpaceOffset = Text.find_last_of(Blanks, SpaceOffset);
 
   if (SpaceOffset == StringRef::npos ||
       // Don't break at leading whitespace.
       Text.find_last_not_of(Blanks, SpaceOffset) == StringRef::npos) {
     // Make sure that we don't break at leading whitespace that
     // reaches past MaxSplit.
     StringRef::size_type FirstNonWhitespace = Text.find_first_not_of(Blanks);
     if (FirstNonWhitespace == StringRef::npos)
       // If the comment is only whitespace, we cannot split.
       return BreakableToken::Split(StringRef::npos, 0);
     SpaceOffset = Text.find_first_of(
         Blanks, std::max<unsigned>(MaxSplitBytes, FirstNonWhitespace));
   }
   if (SpaceOffset != StringRef::npos && SpaceOffset != 0) {
+    // adaptStartOfLine will break after lines starting with /** if the comment
+    // is broken anywhere. Avoid emitting this break twice here.
+    // Example: in /** longtextcomesherethatbreaks */ (with ColumnLimit 20) will
+    // insert a break after /**, so this code must not insert the same break.
+    if (SpaceOffset == 1 && Text[SpaceOffset - 1] == '*')
+      return BreakableToken::Split(StringRef::npos, 0);
     StringRef BeforeCut = Text.substr(0, SpaceOffset).rtrim(Blanks);
     StringRef AfterCut = Text.substr(SpaceOffset).ltrim(Blanks);
     return BreakableToken::Split(BeforeCut.size(),
                                  AfterCut.begin() - BeforeCut.end());
   }
   return BreakableToken::Split(StringRef::npos, 0);
 }
 

 BreakableComment::getSplit(unsigned LineIndex, unsigned TailOffset,
                            unsigned ColumnLimit, unsigned ContentStartColumn,
                            llvm::Regex &CommentPragmasRegex) const {
   // Don't break lines matching the comment pragmas regex.
   if (CommentPragmasRegex.match(Content[LineIndex]))
     return Split(StringRef::npos, 0);
   return getCommentSplit(Content[LineIndex].substr(TailOffset),
                          ContentStartColumn, ColumnLimit, Style.TabWidth,
-                         Encoding);
+                         Encoding, Style);
 }
 
 void BreakableComment::compressWhitespace(
     unsigned LineIndex, unsigned TailOffset, Split Split,
     WhitespaceManager &Whitespaces) const {
   StringRef Text = Content[LineIndex].substr(TailOffset);
   // Text is relative to the content line, but Whitespaces operates relative to
   // the start of the corresponding token, so compute the start of the Split

 }
 
 void BreakableBlockComment::adaptStartOfLine(
     unsigned LineIndex, WhitespaceManager &Whitespaces) const {
   if (LineIndex == 0) {
     if (DelimitersOnNewline) {
       // Since we're breaking at index 1 below, the break position and the
       // break length are the same.
+      // Note: this works because getCommentSplit is careful never to split at
+      // the beginning of a line.
       size_t BreakLength = Lines[0].substr(1).find_first_not_of(Blanks);
       if (BreakLength != StringRef::npos)
         insertBreak(LineIndex, 0, Split(1, BreakLength), /*ContentIndent=*/0,
                     Whitespaces);
     }
     return;
   }
   // Here no reflow with the previous line will happen.

cfe/trunk/unittests/Format/FormatTestComments.cpp

EXPECT_EQ("/**\n"
             " *\n"
             " */",
             format("/**\n"
                    " * multiline block comment\n"
                    " *\n"
                    " */",
                    getLLVMStyleWithColumns(20)));
 
+  // This reproduces a crashing bug where both adaptStartOfLine and
+  // getCommentSplit were trying to wrap after the "/**".
+  EXPECT_EQ("/** multilineblockcommentwithnowrapopportunity */",
+            format("/** multilineblockcommentwithnowrapopportunity */",
+                   getLLVMStyleWithColumns(20)));
+
   EXPECT_EQ("/*\n"
             "\n"
             "\n"
             "    */\n",
             format("  /*       \n"
                    "      \n"
                    "               \n"
                    "      */\n"));

cfe/trunk/unittests/Format/FormatTestJS.cpp

TEST_F(FormatTestJS, JSDocComments) {
 
   // Don't break the first line of a single line short jsdoc comment pragma.
   EXPECT_EQ("/** @returns j */",
             format("/** @returns j */",
                    getGoogleJSStyleWithColumns(20)));
 
   // Break a single line long jsdoc comment pragma.
   EXPECT_EQ("/**\n"
-            " * @returns\n"
-            " *     {string}\n"
-            " *     jsdoc line 12\n"
+            " * @returns {string} jsdoc line 12\n"
             " */",
             format("/** @returns {string} jsdoc line 12 */",
                    getGoogleJSStyleWithColumns(20)));
-
   EXPECT_EQ("/**\n"
-            " * @returns\n"
-            " *     {string}\n"
+            " * @returns {string}\n"
             " *     jsdoc line 12\n"
             " */",
-            format("/** @returns {string} jsdoc line 12  */",
+            format("/** @returns {string} jsdoc line 12 */",
+                   getGoogleJSStyleWithColumns(25)));
+
+  EXPECT_EQ("/**\n"
+            " * @returns {string} jsdoc line 12\n"
+            " */",
+            format("/** @returns {string} jsdoc line 12  */",
                    getGoogleJSStyleWithColumns(20)));
 
   // FIXME: this overcounts the */ as a continuation of the 12 when breaking.
   // Related to the FIXME in BreakableBlockComment::getRangeLength.
   EXPECT_EQ("/**\n"
-            " * @returns\n"
-            " *     {string}\n"
-            " *     jsdoc line\n"
+            " * @returns {string}\n"
+            " *     jsdoc line line\n"
             " *     12\n"
             " */",
-            format("/** @returns {string} jsdoc line 12*/",
-                   getGoogleJSStyleWithColumns(20)));
+            format("/** @returns {string} jsdoc line line 12*/",
+                   getGoogleJSStyleWithColumns(25)));
 
   // Fix a multiline jsdoc comment ending in a comment pragma.
   EXPECT_EQ("/**\n"
             " * line 1\n"
             " * line 2\n"
             " * @returns {string}\n"
             " *     jsdoc line 12\n"
             " */",
verifyFormat("xxxxxxxxxxx(\n"
                "    });",
                getGoogleJSStyleWithColumns(40));
   verifyFormat("while (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n"
                "       bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb) {\n}");
 }
 
 TEST_F(FormatTestJS, JSDocAnnotations) {
   verifyFormat("/**\n"
-               " * @exports\n"
-               " *     {this.is.a.long.path.to.a.Type}\n"
+               " * @exports {this.is.a.long.path.to.a.Type}\n"
                " */",
                "/**\n"
                " * @exports {this.is.a.long.path.to.a.Type}\n"
                " */",
                getGoogleJSStyleWithColumns(20));
   verifyFormat("/**\n"
-               " * @mods\n"
-               " *     {this.is.a.long.path.to.a.Type}\n"
+               " * @mods {this.is.a.long.path.to.a.Type}\n"
                " */",
                "/**\n"
                " * @mods {this.is.a.long.path.to.a.Type}\n"
                " */",
                getGoogleJSStyleWithColumns(20));
   verifyFormat("/**\n"
-               " * @param\n"
-               " *     {this.is.a.long.path.to.a.Type}\n"
+               " * @mods {this.is.a.long.path.to.a.Type}\n"
                " */",
                "/**\n"
-               " * @param {this.is.a.long.path.to.a.Type}\n"
+               " * @mods {this.is.a.long.path.to.a.Type}\n"
+               " */",
+               getGoogleJSStyleWithColumns(20));
+  verifyFormat("/**\n"
+               " * @param {canWrap\n"
+               " *     onSpace}\n"
+               " */",
+               "/**\n"
+               " * @param {canWrap onSpace}\n"
                " */",
                getGoogleJSStyleWithColumns(20));
   verifyFormat("/**\n"
                " * @see http://very/very/long/url/is/long\n"
                " */",
                "/**\n"
                " * @see http://very/very/long/url/is/long\n"
                " */",
                getGoogleJSStyleWithColumns(20));
   verifyFormat(
       "/**\n"
       " * @param This is a\n"
       " *     long comment\n"
       " *     but no type\n"
       " */",
       "/**\n"
       " * @param This is a long comment but no type\n"
       " */",
       getGoogleJSStyleWithColumns(20));
   // Break and reindent @param line and reflow unrelated lines.
   EXPECT_EQ("{\n"
             "  /**\n"
             "   * long long long\n"
             "   * long\n"
-            "   * @param\n"
-            "   *     {this.is.a.long.path.to.a.Type}\n"
+            "   * @param {this.is.a.long.path.to.a.Type}\n"
             "   *     a\n"
             "   * long long long\n"
             "   * long long\n"
             "   */\n"
             "  function f(a) {}\n"
             "}",
             format("{\n"
                    "/**\n"