[clang-format] Put '/**' and '*/' on own lines in jsdocs ending in comment pragmas

[krasimir]

Summary

This handles a case where the trailing '*/' of a multiline jsdoc eding in a
comment pragma wouldn't be put on a new line.

[mprobst]

lib/Format/BreakableToken.cpp

688 ↗

(On Diff #109877)

Can you add a comment on what this is doing? It's non obvious. Make sure to document both intention (why) and implementation (how).

688 ↗

(On Diff #109877)

I don't understand why we're trimming Content.back() here, and Lines.back() below.

690 ↗

(On Diff #109877)

Whitespaces seems like a bad variable name, isn't this rather the index of trimmed whitespace in the last line?

[krasimir]

• Simplify getSplitAfterLastLine

[krasimir]

• Remove debugging

cfe/trunk/lib/Format/BreakableToken.h

virtual void replaceWhitespaceBefore(unsigned LineIndex,
                                        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 {
+  virtual Split getSplitAfterLastLine(unsigned TailOffset,
+                                      unsigned ColumnLimit) 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,
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;
+  Split getSplitAfterLastLine(unsigned TailOffset,
+                              unsigned ColumnLimit) 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

cfe/trunk/lib/Format/BreakableToken.cpp

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);
+BreakableToken::Split
+BreakableBlockComment::getSplitAfterLastLine(unsigned TailOffset,
+                                             unsigned ColumnLimit) const {
+  if (DelimitersOnNewline) {
+    // Replace the trailing whitespace of the last line with a newline.
+    // In case the last line is empty, the ending '*/' is already on its own
+    // line.
+    StringRef Line = Content.back().substr(TailOffset);
+    StringRef TrimmedLine = Line.rtrim(Blanks);
+    if (!TrimmedLine.empty())
+      return Split(TrimmedLine.size(), Line.size() - TrimmedLine.size());
+  }
   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];

cfe/trunk/lib/Format/ContinuationIndenter.cpp

while (RemainingTokenColumns > RemainingSpace) {
       }
       TailOffset += Split.first + Split.second;
       RemainingTokenColumns = NewRemainingTokenColumns;
       ReflowInProgress = true;
       BreakInserted = true;
     }
   }
 
-  BreakableToken::Split SplitAfterLastLine = Token->getSplitAfterLastLine(
-      TailOffset, ColumnLimit, CommentPragmasRegex);
+  BreakableToken::Split SplitAfterLastLine =
+      Token->getSplitAfterLastLine(TailOffset, ColumnLimit);
   if (SplitAfterLastLine.first != StringRef::npos) {
     if (!DryRun)
       Token->replaceWhitespaceAfterLastLine(TailOffset, SplitAfterLastLine,
                                             Whitespaces);
     RemainingTokenColumns = Token->getLineLengthAfterSplitAfterLastLine(
         TailOffset, SplitAfterLastLine);
   }
 

cfe/trunk/unittests/Format/FormatTestJS.cpp

EXPECT_EQ("function f() {\n"
             "   */\n"
             "  var x = 1;\n"
             "}",
             format("function f() {\n"
                    "/** comment about x */\n"
                    "var x = 1;\n"
                    "}",
                    getGoogleJSStyleWithColumns(20)));
+
+  // 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 {string} jsdoc line 12\n"
+            " */",
+            format("/** @returns {string} jsdoc line 12 */",
+                   getGoogleJSStyleWithColumns(20)));
+
+  EXPECT_EQ("/**\n"
+            " * @returns {string} jsdoc line 12\n"
+            " */",
+            format("/** @returns {string} jsdoc line 12  */",
+                   getGoogleJSStyleWithColumns(20)));
+
+  EXPECT_EQ("/**\n"
+            " * @returns {string} jsdoc line 12\n"
+            " */",
+            format("/** @returns {string} jsdoc line 12*/",
+                   getGoogleJSStyleWithColumns(20)));
+
+  // Fix a multiline jsdoc comment ending in a comment pragma.
+  EXPECT_EQ("/**\n"
+            " * line 1\n"
+            " * line 2\n"
+            " * @returns {string} jsdoc line 12\n"
+            " */",
+            format("/** line 1\n"
+                   " * line 2\n"
+                   " * @returns {string} jsdoc line 12 */",
+                   getGoogleJSStyleWithColumns(20)));
+
+  EXPECT_EQ("/**\n"
+            " * line 1\n"
+            " * line 2\n"
+            " *\n"
+            " * @returns j\n"
+            " */",
+            format("/** line 1\n"
+                   " * line 2\n"
+                   " *\n"
+                   " * @returns j */",
+                   getGoogleJSStyleWithColumns(20)));
 }
 
 TEST_F(FormatTestJS, UnderstandsJavaScriptOperators) {
   verifyFormat("a == = b;");
   verifyFormat("a != = b;");
 
   verifyFormat("a === b;");
   verifyFormat("aaaaaaa ===\n    b;", getGoogleJSStyleWithColumns(10));