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

[Krasimir Georgiev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL310458: [clang-format] Put '/**' and '*/' on own lines in 
jsdocs ending in comment… (authored by krasimir).

Repository:
  rL LLVM

https://reviews.llvm.org/D36359

Files:
  cfe/trunk/lib/Format/BreakableToken.cpp
  cfe/trunk/lib/Format/BreakableToken.h
  cfe/trunk/lib/Format/ContinuationIndenter.cpp
  cfe/trunk/unittests/Format/FormatTestJS.cpp

Index: cfe/trunk/lib/Format/BreakableToken.h
===
--- cfe/trunk/lib/Format/BreakableToken.h
+++ cfe/trunk/lib/Format/BreakableToken.h
@@ -161,8 +161,8 @@
   ///
   /// 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);
   }
 
@@ -347,8 +347,8 @@
   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;
Index: cfe/trunk/lib/Format/BreakableToken.cpp
===
--- cfe/trunk/lib/Format/BreakableToken.cpp
+++ cfe/trunk/lib/Format/BreakableToken.cpp
@@ -681,12 +681,18 @@
   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);
 }
 
Index: cfe/trunk/lib/Format/ContinuationIndenter.cpp
===
--- cfe/trunk/lib/Format/ContinuationIndenter.cpp
+++ cfe/trunk/lib/Format/ContinuationIndenter.cpp
@@ -1383,8 +1383,8 @@
 }
   }
 
-  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,
Index: cfe/trunk/unittests/Format/FormatTestJS.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJS.cpp
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp
@@ -158,6 +158,53 @@
"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"
+

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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir updated this revision to Diff 110157.
krasimir added a comment.

- Remove debugging


https://reviews.llvm.org/D36359

Files:
  lib/Format/BreakableToken.cpp
  lib/Format/BreakableToken.h
  lib/Format/ContinuationIndenter.cpp
  unittests/Format/FormatTestJS.cpp

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -158,6 +158,53 @@
"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) {
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1383,8 +1383,8 @@
 }
   }
 
-  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,
Index: lib/Format/BreakableToken.h
===
--- lib/Format/BreakableToken.h
+++ lib/Format/BreakableToken.h
@@ -161,8 +161,8 @@
   ///
   /// 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);
   }
 
@@ -347,8 +347,8 @@
   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;
Index: lib/Format/BreakableToken.cpp
===
--- lib/Format/BreakableToken.cpp
+++ lib/Format/BreakableToken.cpp
@@ -681,12 +681,18 @@
   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

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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir updated this revision to Diff 110156.
krasimir added a comment.

- Simplify getSplitAfterLastLine


https://reviews.llvm.org/D36359

Files:
  lib/Format/BreakableToken.cpp
  lib/Format/BreakableToken.h
  lib/Format/ContinuationIndenter.cpp
  unittests/Format/FormatTestJS.cpp

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -158,6 +158,53 @@
"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) {
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1383,8 +1383,8 @@
 }
   }
 
-  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,
Index: lib/Format/BreakableToken.h
===
--- lib/Format/BreakableToken.h
+++ lib/Format/BreakableToken.h
@@ -161,8 +161,8 @@
   ///
   /// 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);
   }
 
@@ -347,8 +347,8 @@
   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;
Index: lib/Format/BreakableToken.cpp
===
--- lib/Format/BreakableToken.cpp
+++ lib/Format/BreakableToken.cpp
@@ -22,6 +22,11 @@
 #include 
 
 #define DEBUG_TYPE "format-token-breaker"
+#define DBG(A) \
+  DEBUG({  \
+llvm::dbgs() << __func__ << ":" << __LINE__ << ":" << #A << " = '" << A\
+ << "'\n"; \
+  });
 
 namespace clang {
 namespace format {
@@ -681,12 +686,18 @@
   InPPDirective, /*Newlines=*/1, ContentColumn[LineIndex] - Prefix.size());
 }
 
-BreakableToken::Split BreakableBlockComment::getSplitAfterLastLine(
-unsigned TailOffset

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

[Martin Probst via Phabricator via cfe-commits]

mprobst added inline comments.



Comment at: lib/Format/BreakableToken.cpp:688
+  if (DelimitersOnNewline) {
+StringRef TrimmedContent = Content.back().substr(TailOffset).rtrim(Blanks);
+if (!TrimmedContent.empty()) {

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



Comment at: lib/Format/BreakableToken.cpp:688
+  if (DelimitersOnNewline) {
+StringRef TrimmedContent = Content.back().substr(TailOffset).rtrim(Blanks);
+if (!TrimmedContent.empty()) {

mprobst wrote:
> Can you add a comment on what this is doing? It's non obvious. Make sure to 
> document both intention (why) and implementation (how).
I don't understand why we're trimming `Content.back()` here, and `Lines.back()` 
below.



Comment at: lib/Format/BreakableToken.cpp:690
+if (!TrimmedContent.empty()) {
+  size_t Whitespaces =
+  Lines.back().size() - Lines.back().rtrim(Blanks).size();

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


https://reviews.llvm.org/D36359



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir created this revision.
Herald added a subscriber: klimek.

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


https://reviews.llvm.org/D36359

Files:
  lib/Format/BreakableToken.cpp
  lib/Format/BreakableToken.h
  lib/Format/ContinuationIndenter.cpp
  unittests/Format/FormatTestJS.cpp

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -158,6 +158,53 @@
"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) {
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1383,8 +1383,8 @@
 }
   }
 
-  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,
Index: lib/Format/BreakableToken.h
===
--- lib/Format/BreakableToken.h
+++ lib/Format/BreakableToken.h
@@ -161,8 +161,8 @@
   ///
   /// 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);
   }
 
@@ -347,8 +347,8 @@
   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;
Index: lib/Format/BreakableToken.cpp
===
--- lib/Format/BreakableToken.cpp
+++ lib/Format/BreakableToken.cpp
@@ -681,12 +681,17 @@
   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) con