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

[krasimir]

• Remove empty test block

[mprobst]

lib/Format/BreakableToken.cpp

435 ↗

(On Diff #107510)

I think I'd also enable this for at least Java. But tentatively I wonder - wouldn't just about every language do it like this for /** comments?

436 ↗

(On Diff #107510)

Wouldn't we also want to do this for /**blah, i.e. no whitespace after *?

436 ↗

(On Diff #107510)

Would we also want to do this for simple block comments, e.g. /*blah*/? That's a bit more tricky as they can be used inline, not sure about the corner cases there.

606 ↗

(On Diff #107510)

isn't this BreakPosition rather than length?

unittests/Format/FormatTestJS.cpp

136 ↗

(On Diff #107510)

do you need a test where the entire comment block is indented?

[krasimir]

• Address review comments

[krasimir]

lib/Format/BreakableToken.cpp

435 ↗

(On Diff #107510)

I agree for Java, but otherwise not. C++ is crazy with strange styles.

436 ↗

(On Diff #107510)

I explicitly check for a whitespace after the * to take care of strange stuff that is not javadoc, like ASCII art or lines like:

/********
 * #yolo
 */

I think we can decide for simple block comments later and leave them as-is for now.

606 ↗

(On Diff #107510)

It's both the BreakPosition and the BreakLength, because we're breaking starting from index 1.
Added a comment line (pun intended).

[mprobst]

lib/Format/BreakableToken.cpp

436 ↗

(On Diff #107510)

Makes sense. Maybe add some negative tests for such situations, to make sure we don't screw up the source?

[krasimir]

• Add negative tests

cfe/trunk/lib/Format/BreakableToken.h

 /// - getSplitBefore, for finding a split such that the content preceding it
 ///   needs to be specially reflown,
 /// - getLineLengthAfterSplitBefore, for calculating the line length in columns
 ///   of the remainder of the content after the beginning of the content has
 ///   been reformatted, and
 /// - 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 {
 public:
   /// \brief Contains starting character index and length of split.
   typedef std::pair<StringRef::size_type, unsigned> Split;
 
   virtual ~BreakableToken() {}
public:
   /// \brief Replaces the whitespace between \p LineIndex-1 and \p LineIndex.
   /// Performs a reformatting of the content at \p LineIndex preceding the
   /// whitespace range \p SplitBefore.
   virtual void replaceWhitespaceBefore(unsigned LineIndex,
                                        unsigned PreviousEndColumn,
                                        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.
   virtual void updateNextToken(LineState &State) const {}
 
 protected:
   BreakableToken(const FormatToken &Tok, bool InPPDirective,
                  encoding::Encoding Encoding, const FormatStyle &Style)
public:
   unsigned getLineLengthAfterSplitBefore(unsigned LineIndex,
                                          unsigned TailOffset,
                                          unsigned PreviousEndColumn,
                                          unsigned ColumnLimit,
                                          Split SplitBefore) const override;
   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;
 
 private:
   // Rearranges the whitespace between Lines[LineIndex-1] and Lines[LineIndex].
   //
   // Updates Content[LineIndex-1] and Content[LineIndex] by stripping off
   // leading and trailing whitespace.
private:
   bool LastLineNeedsDecoration;
 
   // Either "* " if all lines begin with a "*", or empty.
   StringRef Decoration;
 
   // 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 {
 public:
   BreakableLineCommentSection(const FormatToken &Token, unsigned StartColumn,
                               unsigned OriginalStartColumn, bool FirstInLine,
                               bool InPPDirective, encoding::Encoding Encoding,
                               const FormatStyle &Style);

cfe/trunk/lib/Format/BreakableToken.cpp

return Content.size() >= 2 && !hasSpecialMeaningPrefix &&
          // true, then the first code point must be 1 byte long.
          (!isPunctuation(Content[0]) || !isPunctuation(Content[1]));
 }
 
 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");
 
   StringRef TokenText(Tok.TokenText);
   assert(TokenText.startswith("/*") && TokenText.endswith("*/"));
   TokenText.substr(2, TokenText.size() - 4).split(Lines, "\n");
 
   int IndentDelta = StartColumn - OriginalStartColumn;
for (size_t i = 1, e = Lines.size(); i < e; ++i) {
     Content[i] = Content[i].substr(DecorationSize);
     if (!Decoration.startswith(Content[i]))
       IndentAtLineBreak =
           std::min<int>(IndentAtLineBreak, std::max(0, ContentColumn[i]));
   }
   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] << "| "
                    << "IN=" << (Content[i].data() - Lines[i].data()) << "\n";
     }
   });
 }
 
if (SplitBefore.first == StringRef::npos ||
     unsigned ReflownColumn =
         getReflownColumn(TrimmedContent, LineIndex, PreviousEndColumn);
     if (ReflownColumn <= ColumnLimit) {
       return ReflownColumn;
     }
     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.
     assert(Tokens[LineIndex - 1] == Tokens[LineIndex] &&
            "Reflowing whitespace within a token");
     // This is the offset of the end of the last line relative to the start of
     // the token text in the token.
     unsigned WhitespaceOffsetInToken = Content[LineIndex - 1].data() +
void BreakableBlockComment::replaceWhitespaceBefore(
   unsigned WhitespaceLength = Content[LineIndex].data() -
                               tokenAt(LineIndex).TokenText.data() -
                               WhitespaceOffsetInToken;
   Whitespaces.replaceWhitespaceInToken(
       tokenAt(LineIndex), WhitespaceOffsetInToken, WhitespaceLength, "", Prefix,
       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
   // case, we compute the start of the comment pragma manually.
   StringRef IndentContent = Content[LineIndex];
   if (Lines[LineIndex].ltrim(Blanks).startswith("*")) {
     IndentContent = Lines[LineIndex].ltrim(Blanks).substr(1);
   }

cfe/trunk/lib/Format/ContinuationIndenter.cpp

unsigned ContinuationIndenter::breakProtrudingToken(const FormatToken &Current,
   unsigned RemainingSpace = ColumnLimit - Current.UnbreakableTailLength;
   bool BreakInserted = false;
   // We use a conservative reflowing strategy. Reflow starts after a line is
   // broken or the corresponding whitespace compressed. Reflow ends as soon as a
   // line that doesn't get reflown with the previous line is reached.
   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);
     if (ReflowInProgress) {
       SplitBefore = Token->getSplitBefore(LineIndex, RemainingTokenColumns,
                                           RemainingSpace, CommentPragmasRegex);
     }
     ReflowInProgress = SplitBefore.first != StringRef::npos;
-    unsigned TailOffset =
+    TailOffset =
         ReflowInProgress ? (SplitBefore.first + SplitBefore.second) : 0;
     if (!DryRun)
       Token->replaceWhitespaceBefore(LineIndex, RemainingTokenColumns,
                                      RemainingSpace, SplitBefore, Whitespaces);
     RemainingTokenColumns = Token->getLineLengthAfterSplitBefore(
         LineIndex, TailOffset, RemainingTokenColumns, ColumnLimit, SplitBefore);
     while (RemainingTokenColumns > RemainingSpace) {
       BreakableToken::Split Split = Token->getSplit(
while (RemainingTokenColumns > RemainingSpace) {
       }
       TailOffset += Split.first + Split.second;
       RemainingTokenColumns = NewRemainingTokenColumns;
       ReflowInProgress = true;
       BreakInserted = true;
     }
   }
 
+  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) {
     // If we break the token inside a parameter list, we need to break before
     // the next parameter on all levels, so that the next parameter is clearly
     // visible. Line comments already introduce a break.
     if (Current.isNot(TT_LineComment)) {
       for (unsigned i = 0, e = State.Stack.size(); i != e; ++i)

cfe/trunk/unittests/Format/FormatTestJS.cpp

protected:
   }
 };
 
 TEST_F(FormatTestJS, BlockComments) {
   verifyFormat("/* aaaaaaaaaaaaa */ aaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
                "    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;");
 
   verifyFormat("a === b;");
   verifyFormat("aaaaaaa ===\n    b;", getGoogleJSStyleWithColumns(10));
   verifyFormat("a !== b;");
   verifyFormat("aaaaaaa !==\n    b;", getGoogleJSStyleWithColumns(10));

cfe/trunk/unittests/Format/FormatTestJava.cpp

EXPECT_EQ("/*\n"
             "void f() {}",
             format("  /*\n"
                    "   * Really multi-line\n"
                    "   * comment.\n"
                    "   */\n"
                    "  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"
                  "  a >>>= 1;\n"
                  "}");
     verifyFormat("void f() {\n"
                  "  int a = 1;\n"
                  "  a = a >>> 1;\n"
                  "}");
 }
 
 
 } // end namespace tooling
 } // end namespace clang