[clang-format] Put '/**' and '*/' on own lines in multiline jsdocs

Reviewers: mprobst

Reviewed By: mprobst

Subscribers: cfe-commits, klimek

Differential Revision: https://reviews.llvm.org/D35683

llvm-svn: 308684

clang/lib/Format/BreakableToken.cpp

@@ -339,7 +339,8 @@ BreakableBlockComment::BreakableBlockComment(
     const FormatToken &Token, unsigned StartColumn,
     unsigned OriginalStartColumn, bool FirstInLine, bool InPPDirective,
     encoding::Encoding Encoding, const FormatStyle &Style)
-    : BreakableComment(Token, StartColumn, InPPDirective, Encoding, Style) {
+    : BreakableComment(Token, StartColumn, InPPDirective, Encoding, Style),
+      DelimitersOnNewline(false) {
   assert(Tok.is(TT_BlockComment) &&
          "block comment section must start with a block comment");
 
@@ -430,8 +431,25 @@ BreakableBlockComment::BreakableBlockComment(
   IndentAtLineBreak =
       std::max<unsigned>(IndentAtLineBreak, Decoration.size());
 
+  // Detect a multiline jsdoc comment and set DelimitersOnNewline in that case.
+  if (Style.Language == FormatStyle::LK_JavaScript ||
+      Style.Language == FormatStyle::LK_Java) {
+    if ((Lines[0] == "*" || Lines[0].startswith("* ")) && Lines.size() > 1) {
+      // This is a multiline jsdoc comment.
+      DelimitersOnNewline = true;
+    } else if (Lines[0].startswith("* ") && Lines.size() == 1) {
+      // Detect a long single-line comment, like:
+      // /** long long long */
+      // Below, '2' is the width of '*/'.
+      unsigned EndColumn = ContentColumn[0] + encoding::columnWidthWithTabs(
+          Lines[0], ContentColumn[0], Style.TabWidth, Encoding) + 2;
+      DelimitersOnNewline = EndColumn > Style.ColumnLimit;
+    }
+  }
+
   DEBUG({
     llvm::dbgs() << "IndentAtLineBreak " << IndentAtLineBreak << "\n";
+    llvm::dbgs() << "DelimitersOnNewline " << DelimitersOnNewline << "\n";
     for (size_t i = 0; i < Lines.size(); ++i) {
       llvm::dbgs() << i << " |" << Content[i] << "| "
                    << "CC=" << ContentColumn[i] << "| "
@@ -580,10 +598,22 @@ unsigned BreakableBlockComment::getLineLengthAfterSplitBefore(
     return getLineLengthAfterSplit(LineIndex, TailOffset, StringRef::npos);
   }
 }
+
 void BreakableBlockComment::replaceWhitespaceBefore(
     unsigned LineIndex, unsigned PreviousEndColumn, unsigned ColumnLimit,
     Split SplitBefore, WhitespaceManager &Whitespaces) {
-  if (LineIndex == 0) return;
+  if (LineIndex == 0) {
+    if (DelimitersOnNewline) {
+        // Since we're breaking af index 1 below, the break position and the
+        // break length are the same.
+        size_t BreakLength = Lines[0].substr(1).find_first_not_of(Blanks);
+        if (BreakLength != StringRef::npos) {
+          insertBreak(LineIndex, 0, Split(1, BreakLength), Whitespaces);
+          DelimitersOnNewline = true;
+        }
+    }
+    return;
+  }
   StringRef TrimmedContent = Content[LineIndex].ltrim(Blanks);
   if (SplitBefore.first != StringRef::npos) {
     // Here we need to reflow.
@@ -651,6 +681,15 @@ void BreakableBlockComment::replaceWhitespaceBefore(
       InPPDirective, /*Newlines=*/1, ContentColumn[LineIndex] - Prefix.size());
 }
 
+BreakableToken::Split BreakableBlockComment::getSplitAfterLastLine(
+    unsigned TailOffset, unsigned ColumnLimit,
+    llvm::Regex &CommentPragmasRegex) const {
+  if (DelimitersOnNewline)
+    return getSplit(Lines.size() - 1, TailOffset, ColumnLimit,
+                    CommentPragmasRegex);
+  return Split(StringRef::npos, 0);
+}
+
 bool BreakableBlockComment::mayReflow(unsigned LineIndex,
                                       llvm::Regex &CommentPragmasRegex) const {
   // Content[LineIndex] may exclude the indent after the '*' decoration. In that

clang/lib/Format/BreakableToken.h

@@ -64,6 +64,17 @@ struct FormatStyle;
 /// - replaceWhitespaceBefore, for executing the reflow using a whitespace
 ///   manager.
 ///
+/// For tokens that require the whitespace after the last line to be
+/// reformatted, for example in multiline jsdoc comments that require the
+/// trailing '*/' to be on a line of itself, there are analogous operations
+/// that might be executed after the last line has been reformatted:
+/// - getSplitAfterLastLine, for finding a split after the last line that needs
+///   to be reflown,
+/// - getLineLengthAfterSplitAfterLastLine, for calculating the line length in
+///   columns of the remainder of the token, and
+/// - replaceWhitespaceAfterLastLine, for executing the reflow using a
+///   whitespace manager.
+///
 /// FIXME: The interface seems set in stone, so we might want to just pull the
 /// strategy into the class, instead of controlling it from the outside.
 class BreakableToken {
@@ -144,6 +155,38 @@ class BreakableToken {
                                        unsigned ColumnLimit, Split SplitBefore,
                                        WhitespaceManager &Whitespaces) {}
 
+  /// \brief Returns a whitespace range (offset, length) of the content at
+  /// the last line that needs to be reformatted after the last line has been
+  /// reformatted.
+  ///
+  /// A result having offset == StringRef::npos means that no reformat is
+  /// necessary.
+  virtual Split getSplitAfterLastLine(unsigned TailOffset, unsigned ColumnLimit,
+                                      llvm::Regex &CommentPragmasRegex) const {
+    return Split(StringRef::npos, 0);
+  }
+
+  /// \brief Returns the number of columns required to format the piece token
+  /// after the last line after a reformat of the whitespace range \p
+  /// \p SplitAfterLastLine on the last line has been performed.
+  virtual unsigned
+  getLineLengthAfterSplitAfterLastLine(unsigned TailOffset,
+                                       Split SplitAfterLastLine) const {
+    return getLineLengthAfterSplit(getLineCount() - 1,
+                                   TailOffset + SplitAfterLastLine.first +
+                                       SplitAfterLastLine.second,
+                                   StringRef::npos);
+  }
+
+  /// \brief Replaces the whitespace from \p SplitAfterLastLine on the last line
+  /// after the last line has been formatted by performing a reformatting.
+  virtual void replaceWhitespaceAfterLastLine(unsigned TailOffset,
+                                              Split SplitAfterLastLine,
+                                              WhitespaceManager &Whitespaces) {
+    insertBreak(getLineCount() - 1, TailOffset, SplitAfterLastLine,
+                Whitespaces);
+  }
+
   /// \brief Updates the next token of \p State to the next token after this
   /// one. This can be used when this token manages a set of underlying tokens
   /// as a unit and is responsible for the formatting of the them.
@@ -304,6 +347,9 @@ class BreakableBlockComment : public BreakableComment {
   void replaceWhitespaceBefore(unsigned LineIndex, unsigned PreviousEndColumn,
                                unsigned ColumnLimit, Split SplitBefore,
                                WhitespaceManager &Whitespaces) override;
+  Split getSplitAfterLastLine(unsigned TailOffset, unsigned ColumnLimit,
+                              llvm::Regex &CommentPragmasRegex) const override;
+
   bool mayReflow(unsigned LineIndex,
                  llvm::Regex &CommentPragmasRegex) const override;
 
@@ -348,6 +394,10 @@ class BreakableBlockComment : public BreakableComment {
   // If this block comment has decorations, this is the column of the start of
   // the decorations.
   unsigned DecorationColumn;
+
+  // If true, make sure that the opening '/**' and the closing '*/' ends on a
+  // line of itself. Styles like jsdoc require this for multiline comments.
+  bool DelimitersOnNewline;
 };
 
 class BreakableLineCommentSection : public BreakableComment {

clang/lib/Format/ContinuationIndenter.cpp

@@ -1314,6 +1314,7 @@ unsigned ContinuationIndenter::breakProtrudingToken(const FormatToken &Current,
   bool ReflowInProgress = false;
   unsigned Penalty = 0;
   unsigned RemainingTokenColumns = 0;
+  unsigned TailOffset = 0;
   for (unsigned LineIndex = 0, EndIndex = Token->getLineCount();
        LineIndex != EndIndex; ++LineIndex) {
     BreakableToken::Split SplitBefore(StringRef::npos, 0);
@@ -1322,7 +1323,7 @@ unsigned ContinuationIndenter::breakProtrudingToken(const FormatToken &Current,
                                           RemainingSpace, CommentPragmasRegex);
     }
     ReflowInProgress = SplitBefore.first != StringRef::npos;
-    unsigned TailOffset =
+    TailOffset =
         ReflowInProgress ? (SplitBefore.first + SplitBefore.second) : 0;
     if (!DryRun)
       Token->replaceWhitespaceBefore(LineIndex, RemainingTokenColumns,
@@ -1379,6 +1380,16 @@ unsigned ContinuationIndenter::breakProtrudingToken(const FormatToken &Current,
     }
   }
 
+  BreakableToken::Split SplitAfterLastLine = Token->getSplitAfterLastLine(
+      TailOffset, ColumnLimit, CommentPragmasRegex);
+  if (SplitAfterLastLine.first != StringRef::npos) {
+    if (!DryRun)
+      Token->replaceWhitespaceAfterLastLine(TailOffset, SplitAfterLastLine,
+                                            Whitespaces);
+    RemainingTokenColumns = Token->getLineLengthAfterSplitAfterLastLine(
+        TailOffset, SplitAfterLastLine);
+  }
+
   State.Column = RemainingTokenColumns;
 
   if (BreakInserted) {

clang/unittests/Format/FormatTestJS.cpp

@@ -67,6 +67,99 @@ TEST_F(FormatTestJS, BlockComments) {
                "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");
 }
 
+TEST_F(FormatTestJS, JSDocComments) {
+  // Break the first line of a multiline jsdoc comment.
+  EXPECT_EQ("/**\n"
+            " * jsdoc line 1\n"
+            " * jsdoc line 2\n"
+            " */",
+            format("/** jsdoc line 1\n"
+                   " * jsdoc line 2\n"
+                   " */",
+                   getGoogleJSStyleWithColumns(20)));
+  // Both break after '/**' and break the line itself.
+  EXPECT_EQ("/**\n"
+            " * jsdoc line long\n"
+            " * long jsdoc line 2\n"
+            " */",
+            format("/** jsdoc line long long\n"
+                   " * jsdoc line 2\n"
+                   " */",
+                   getGoogleJSStyleWithColumns(20)));
+  // Break a short first line if the ending '*/' is on a newline.
+  EXPECT_EQ("/**\n"
+            " * jsdoc line 1\n"
+            " */",
+            format("/** jsdoc line 1\n"
+                   " */", getGoogleJSStyleWithColumns(20)));
+  // Don't break the first line of a short single line jsdoc comment.
+  EXPECT_EQ("/** jsdoc line 1 */",
+            format("/** jsdoc line 1 */", getGoogleJSStyleWithColumns(20)));
+  // Don't break the first line of a single line jsdoc comment if it just fits
+  // the column limit.
+  EXPECT_EQ("/** jsdoc line 12 */",
+            format("/** jsdoc line 12 */", getGoogleJSStyleWithColumns(20)));
+  // Don't break after '/**' and before '*/' if there is no space between
+  // '/**' and the content.
+  EXPECT_EQ(
+      "/*** nonjsdoc long\n"
+      " * line */",
+      format("/*** nonjsdoc long line */", getGoogleJSStyleWithColumns(20)));
+  EXPECT_EQ(
+      "/**strange long long\n"
+      " * line */",
+      format("/**strange long long line */", getGoogleJSStyleWithColumns(20)));
+  // Break the first line of a single line jsdoc comment if it just exceeds the
+  // column limit.
+  EXPECT_EQ("/**\n"
+            " * jsdoc line 123\n"
+            " */",
+            format("/** jsdoc line 123 */", getGoogleJSStyleWithColumns(20)));
+  // Break also if the leading indent of the first line is more than 1 column.
+  EXPECT_EQ("/**\n"
+            " * jsdoc line 123\n"
+            " */",
+            format("/**  jsdoc line 123 */", getGoogleJSStyleWithColumns(20)));
+  // Break also if the leading indent of the first line is more than 1 column.
+  EXPECT_EQ("/**\n"
+            " * jsdoc line 123\n"
+            " */",
+            format("/**   jsdoc line 123 */", getGoogleJSStyleWithColumns(20)));
+  // Break after the content of the last line.
+  EXPECT_EQ("/**\n"
+            " * line 1\n"
+            " * line 2\n"
+            " */",
+            format("/**\n"
+                   " * line 1\n"
+                   " * line 2 */",
+                   getGoogleJSStyleWithColumns(20)));
+  // Break both the content and after the content of the last line.
+  EXPECT_EQ("/**\n"
+            " * line 1\n"
+            " * line long long\n"
+            " * long\n"
+            " */",
+            format("/**\n"
+                   " * line 1\n"
+                   " * line long long long */",
+                   getGoogleJSStyleWithColumns(20)));
+
+  // The comment block gets indented.
+  EXPECT_EQ("function f() {\n"
+            "  /**\n"
+            "   * comment about\n"
+            "   * x\n"
+            "   */\n"
+            "  var x = 1;\n"
+            "}",
+            format("function f() {\n"
+                   "/** comment about x */\n"
+                   "var x = 1;\n"
+                   "}",
+                   getGoogleJSStyleWithColumns(20)));
+}
+
 TEST_F(FormatTestJS, UnderstandsJavaScriptOperators) {
   verifyFormat("a == = b;");
   verifyFormat("a != = b;");

clang/unittests/Format/FormatTestJava.cpp

@@ -525,6 +525,15 @@ TEST_F(FormatTestJava, AlignsBlockComments) {
                    "  void f() {}"));
 }
 
+TEST_F(FormatTestJava, KeepsDelimitersOnOwnLineInJavaDocComments) {
+  EXPECT_EQ("/**\n"
+            " * javadoc line 1\n"
+            " * javadoc line 2\n"
+            " */",
+            format("/** javadoc line 1\n"
+                   " * javadoc line 2 */"));
+}
+
 TEST_F(FormatTestJava, RetainsLogicalShifts) {
     verifyFormat("void f() {\n"
                  "  int a = 1;\n"