[clang-format] Don't reflow across comment pragmas.

[krasimir]

Summary

The comment reflower wasn't taking comment pragmas as reflow stoppers. This patch fixes that.

source:

// long long long long
// IWYU pragma:

format with column limit = 20 before:

// long long long
// long IWYU pragma:

format with column limit = 20 after:

// long long long
// long 
// IWYU pragma:

[djasper]

I am bit unsure about the design here. Could we instead match against the CommentPragmas and then not even create a BreakableToken (or reflow) if that matches?

I guess that would make us unable to reflow if only part of the comment is a pragma, but that seems ok (for now at least).

[krasimir]

That sounds reasonable. However it won't work with comment pragmas in newlines in block comments.
The root issue that I was looking at was a taze directive inside a block comment in a random typescript file, like:

/* random stuff
  * long long long long
  * taze directive
  */

[krasimir]

Ideally, we may try doing both: break a line comment section token steam and keep this too. What do you think?

[djasper]

Makes sense.

[krasimir]

• Add a break in line comment sections by pragmas

[krasimir]

• Merge branch 'master' into arcpatch-D29450_1

lib/Format/BreakableToken.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_FORMAT_BREAKABLETOKEN_H
 #define LLVM_CLANG_LIB_FORMAT_BREAKABLETOKEN_H
 
 #include "Encoding.h"
 #include "TokenAnnotator.h"
 #include "WhitespaceManager.h"
+#include "llvm/Support/Regex.h"
 #include <utility>
 
 namespace clang {
 namespace format {
 
 /// \brief Checks if \p Token switches formatting, like /* clang-format off */.
 /// \p Token must be a comment.
 bool switchesFormatting(const FormatToken &Token);
public:
   ///
   /// \p PreviousEndColumn is the end column of the previous line after
   /// formatting.
   ///
   /// A result having offset == StringRef::npos means that no piece of the line
   /// needs to be reformatted before any breaks are made.
   virtual Split getSplitBefore(unsigned LineIndex,
                                unsigned PreviousEndColumn,
-                               unsigned ColumnLimit) const {
+                               unsigned ColumnLimit,
+                               llvm::Regex& CommentPragmasRegex) const {
     return Split(StringRef::npos, 0);
   }
 
   /// \brief Returns the number of columns required to format the piece of line
   /// at \p LineIndex after the content preceding the whitespace range specified
   /// \p SplitBefore has been reformatted, but before any breaks are made to
   /// this line.
   virtual unsigned getLineLengthAfterSplitBefore(unsigned LineIndex,
protected:
   Split getReflowSplit(StringRef Text, StringRef ReflowPrefix,
                        unsigned PreviousEndColumn, unsigned ColumnLimit) const;
 
   // Returns the token containing the line at LineIndex.
   const FormatToken &tokenAt(unsigned LineIndex) const;
 
   // Checks if the content of line LineIndex may be reflown with the previous
   // line.
-  bool mayReflow(unsigned LineIndex) const;
+  virtual bool mayReflow(unsigned LineIndex,
+                         llvm::Regex &CommentPragmasRegex) const = 0;
 
   // Contains the original text of the lines of the block comment.
   //
   // In case of a block comments, excludes the leading /* in the first line and
   // trailing */ in the last line. In case of line comments, excludes the
   // leading // and spaces.
   SmallVector<StringRef, 16> Lines;
 
BreakableBlockComment(const FormatToken &Token, unsigned StartColumn,
                         const FormatStyle &Style);
 
   unsigned getLineLengthAfterSplit(unsigned LineIndex,
                                    unsigned TailOffset,
                                    StringRef::size_type Length) const override;
   void insertBreak(unsigned LineIndex, unsigned TailOffset, Split Split,
                    WhitespaceManager &Whitespaces) override;
   Split getSplitBefore(unsigned LineIndex, unsigned PreviousEndColumn,
-                       unsigned ColumnLimit) const override;
+                       unsigned ColumnLimit,
+                       llvm::Regex &CommentPragmasRegex) const override;
   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;
+  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.
   //
   // Sets ContentColumn to the intended column in which the text at
BreakableLineCommentSection(const FormatToken &Token, unsigned StartColumn,
                               const FormatStyle &Style);
 
   unsigned getLineLengthAfterSplit(unsigned LineIndex,
                                    unsigned TailOffset,
                                    StringRef::size_type Length) const override;
   void insertBreak(unsigned LineIndex, unsigned TailOffset, Split Split,
                    WhitespaceManager &Whitespaces) override;
   Split getSplitBefore(unsigned LineIndex, unsigned PreviousEndColumn,
-                       unsigned ColumnLimit) const override;
+                       unsigned ColumnLimit,
+                       llvm::Regex &CommentPragmasRegex) const override;
   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;
   void updateNextToken(LineState& State) const override;
+  bool mayReflow(unsigned LineIndex,
+                 llvm::Regex &CommentPragmasRegex) const override;
 
 private:
   unsigned getContentStartColumn(unsigned LineIndex,
                                  unsigned TailOffset) const override;
 
   // Prefix[i] contains the intended leading "//" with trailing spaces to
   // account for the indentation of content within the comment at line i after
   // formatting. It can be different than the original prefix when the original

lib/Format/BreakableToken.cpp

static bool mayReflowContent(StringRef Content) {
   // non-punctuation.
   return Content.size() >= 2 && !hasSpecialMeaningPrefix &&
          !Content.endswith("\\") &&
          // Note that this is UTF-8 safe, since if isPunctuation(Content[0]) is
          // true, then the first code point must be 1 byte long.
          (!isPunctuation(Content[0]) || !isPunctuation(Content[1]));
 }
 
-bool BreakableComment::mayReflow(unsigned LineIndex) const {
-  return LineIndex > 0 && mayReflowContent(Content[LineIndex]) &&
-         !Tok.Finalized && !switchesFormatting(tokenAt(LineIndex)) &&
-         (!Tok.is(TT_LineComment) ||
-          OriginalPrefix[LineIndex] == OriginalPrefix[LineIndex - 1]);
-}
-
 BreakableBlockComment::BreakableBlockComment(
     const FormatToken &Token, unsigned StartColumn,
     unsigned OriginalStartColumn, bool FirstInLine, bool InPPDirective,
     encoding::Encoding Encoding, const FormatStyle &Style)
     : BreakableComment(Token, StartColumn, OriginalStartColumn, FirstInLine,
                        InPPDirective, Encoding, Style) {
   assert(Tok.is(TT_BlockComment) &&
          "block comment section must start with a block comment");
Whitespaces.replaceWhitespaceInToken(
       tokenAt(LineIndex), BreakOffsetInToken, CharsToRemove, "", Prefix,
       InPPDirective, /*Newlines=*/1,
       /*Spaces=*/LocalIndentAtLineBreak - Prefix.size());
 }
 
 BreakableToken::Split BreakableBlockComment::getSplitBefore(
     unsigned LineIndex,
     unsigned PreviousEndColumn,
-    unsigned ColumnLimit) const {
-  if (!mayReflow(LineIndex))
+    unsigned ColumnLimit,
+    llvm::Regex &CommentPragmasRegex) const {
+  if (!mayReflow(LineIndex, CommentPragmasRegex))
     return Split(StringRef::npos, 0);
   StringRef TrimmedContent = Content[LineIndex].ltrim(Blanks);
   return getReflowSplit(TrimmedContent, ReflowPrefix, PreviousEndColumn,
                         ColumnLimit);
 }
 
 unsigned BreakableBlockComment::getReflownColumn(
     StringRef Content,
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());
 }
 
+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);
+  }
+  return LineIndex > 0 && !CommentPragmasRegex.match(IndentContent) &&
+         mayReflowContent(Content[LineIndex]) && !Tok.Finalized &&
+         !switchesFormatting(tokenAt(LineIndex));
+}
+
 unsigned
 BreakableBlockComment::getContentStartColumn(unsigned LineIndex,
                                              unsigned TailOffset) const {
   // If we break, we always break at the predefined indent.
   if (TailOffset != 0)
     return IndentAtLineBreak;
   return std::max(0, ContentColumn[LineIndex]);
 }
void BreakableLineCommentSection::insertBreak(unsigned LineIndex,
   assert(IndentAtLineBreak >= Prefix[LineIndex].size());
   Whitespaces.replaceWhitespaceInToken(
       tokenAt(LineIndex), BreakOffsetInToken, CharsToRemove, "",
       Prefix[LineIndex], InPPDirective, /*Newlines=*/1,
       /*Spaces=*/IndentAtLineBreak - Prefix[LineIndex].size());
 }
 
 BreakableComment::Split BreakableLineCommentSection::getSplitBefore(
-    unsigned LineIndex,
-    unsigned PreviousEndColumn,
-    unsigned ColumnLimit) const {
-  if (!mayReflow(LineIndex)) return Split(StringRef::npos, 0);
+    unsigned LineIndex, unsigned PreviousEndColumn, unsigned ColumnLimit,
+    llvm::Regex &CommentPragmasRegex) const {
+  if (!mayReflow(LineIndex, CommentPragmasRegex))
+    return Split(StringRef::npos, 0);
   return getReflowSplit(Content[LineIndex], ReflowPrefix, PreviousEndColumn,
                         ColumnLimit);
 }
 
 unsigned BreakableLineCommentSection::getLineLengthAfterSplitBefore(
     unsigned LineIndex, unsigned TailOffset,
     unsigned PreviousEndColumn,
     unsigned ColumnLimit,

 }
 
 void BreakableLineCommentSection::updateNextToken(LineState& State) const {
   if (LastLineTok) {
     State.NextToken = LastLineTok->Next;
   }
 }
 
+bool BreakableLineCommentSection::mayReflow(
+    unsigned LineIndex, llvm::Regex &CommentPragmasRegex) const {
+  // Line comments have the indent as part of the prefix, so we need to
+  // recompute the start of the line.
+  StringRef IndentContent = Content[LineIndex];
+  if (Lines[LineIndex].startswith("//")) {
+    IndentContent = Lines[LineIndex].substr(2);
+  }
+  return LineIndex > 0 && !CommentPragmasRegex.match(IndentContent) &&
+         mayReflowContent(Content[LineIndex]) && !Tok.Finalized &&
+         !switchesFormatting(tokenAt(LineIndex)) &&
+         OriginalPrefix[LineIndex] == OriginalPrefix[LineIndex - 1];
+}
+
 unsigned
 BreakableLineCommentSection::getContentStartColumn(unsigned LineIndex,
                                                    unsigned TailOffset) const {
   if (TailOffset != 0) {
     return OriginalContentColumn[LineIndex];
   }
   return ContentColumn[LineIndex];
 }
 
 } // namespace format
 } // namespace clang

lib/Format/ContinuationIndenter.cpp

unsigned ContinuationIndenter::breakProtrudingToken(const FormatToken &Current,
   bool ReflowInProgress = false;
   unsigned Penalty = 0;
   unsigned RemainingTokenColumns = 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);
+                                          RemainingSpace, CommentPragmasRegex);
     }
     ReflowInProgress = SplitBefore.first != StringRef::npos;
     unsigned TailOffset =
         ReflowInProgress ? (SplitBefore.first + SplitBefore.second) : 0;
     if (!DryRun)
       Token->replaceWhitespaceBefore(LineIndex, RemainingTokenColumns,
                                      RemainingSpace, SplitBefore, Whitespaces);
     RemainingTokenColumns = Token->getLineLengthAfterSplitBefore(

lib/Format/UnwrappedLineParser.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_FORMAT_UNWRAPPEDLINEPARSER_H
 #define LLVM_CLANG_LIB_FORMAT_UNWRAPPEDLINEPARSER_H
 
 #include "FormatToken.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Format/Format.h"
+#include "llvm/Support/Regex.h"
 #include <list>
 #include <stack>
 
 namespace clang {
 namespace format {
 
 struct UnwrappedLineNode;
 
private:
   SmallVectorImpl<UnwrappedLine> *CurrentLines;
 
   // We store for each line whether it must be a declaration depending on
   // whether we are in a compound statement or not.
   std::vector<bool> DeclarationScopeStack;
 
   const FormatStyle &Style;
   const AdditionalKeywords &Keywords;
   
+  llvm::Regex CommentPragmasRegex;
+
   FormatTokenSource *Tokens;
   UnwrappedLineConsumer &Callback;
 
   // FIXME: This is a temporary measure until we have reworked the ownership
   // of the format tokens. The goal is to have the actual tokens created and
   // owned outside of and handed into the UnwrappedLineParser.
   ArrayRef<FormatToken *> AllTokens;
 

lib/Format/UnwrappedLineParser.cpp

 
 } // end anonymous namespace
 
 UnwrappedLineParser::UnwrappedLineParser(const FormatStyle &Style,
                                          const AdditionalKeywords &Keywords,
                                          ArrayRef<FormatToken *> Tokens,
                                          UnwrappedLineConsumer &Callback)
     : Line(new UnwrappedLine), MustBreakBeforeNextToken(false),
-      CurrentLines(&Lines), Style(Style), Keywords(Keywords), Tokens(nullptr),
+      CurrentLines(&Lines), Style(Style), Keywords(Keywords),
+      CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
       Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1) {}
 
 void UnwrappedLineParser::reset() {
   PPBranchLevel = -1;
   Line.reset(new UnwrappedLine);
   CommentsBeforeNextToken.clear();
   FormatTok = nullptr;
   MustBreakBeforeNextToken = false;

 static bool isLineComment(const FormatToken &FormatTok) {
   return FormatTok.is(tok::comment) &&
          FormatTok.TokenText.startswith("//");
 }
 
 // Checks if \p FormatTok is a line comment that continues the line comment
 // section on \p Line.
 static bool continuesLineComment(const FormatToken &FormatTok,
-                                 const UnwrappedLine &Line) {
+                                 const UnwrappedLine &Line,
+                                 llvm::Regex &CommentPragmasRegex) {
   if (Line.Tokens.empty())
     return false;
 
+  StringRef IndentContent = FormatTok.TokenText;
+  if (FormatTok.TokenText.startswith("//") ||
+      FormatTok.TokenText.startswith("/*"))
+    IndentContent = FormatTok.TokenText.substr(2);
+  if (CommentPragmasRegex.match(IndentContent))
+    return false;
+
   // If Line starts with a line comment, then FormatTok continues the comment
   // section if its original column is greater or equal to the original start
   // column of the line.
   //
   // Define the min column token of a line as follows: if a line ends in '{' or
   // contains a '{' followed by a line comment, then the min column token is
   // that '{'. Otherwise, the min column token of the line is the first token of
   // the line.
   //
   // If Line starts with a token other than a line comment, then FormatTok
   // continues the comment section if its original column is greater than the
   // original start column of the min column token of the line.
   //
   // For example, the second line comment continues the first in these cases:
+  //
   // // first line
   // // second line
+  //
   // and:
+  //
   // // first line
   //  // second line
+  //
   // and:
+  //
   // int i; // first line
   //  // second line
+  //
   // and:
+  //
   // do { // first line
   //      // second line
   //   int i;
   // } while (true);
   //
+  // and:
+  //
+  // enum {
+  //   a, // first line
+  //    // second line
+  //   b
+  // };
+  //
   // The second line comment doesn't continue the first in these cases:
+  //
   //   // first line
   //  // second line
+  //
   // and:
+  //
   // int i; // first line
   // // second line
+  //
   // and:
+  //
   // do { // first line
   //   // second line
   //   int i;
   // } while (true);
+  //
+  // and:
+  //
+  // enum {
+  //   a, // first line
+  //   // second line
+  // };
   const FormatToken *MinColumnToken = Line.Tokens.front().Tok;
 
   // Scan for '{//'. If found, use the column of '{' as a min column for line
   // comment section continuation.
   const FormatToken *PreviousToken = nullptr;
   for (const UnwrappedLineNode Node : Line.Tokens) {
     if (PreviousToken && PreviousToken->is(tok::l_brace) &&
         isLineComment(*Node.Tok)) {
       MinColumnToken = PreviousToken;
       break;
     }
     PreviousToken = Node.Tok;
+
+    // Grab the last newline preceding a token in this unwrapped line.
+    if (Node.Tok->NewlinesBefore > 0) {
+      MinColumnToken = Node.Tok;
+    }
   }
   if (PreviousToken && PreviousToken->is(tok::l_brace)) {
     MinColumnToken = PreviousToken;
   }
 
   unsigned MinContinueColumn =
       MinColumnToken->OriginalColumn +
       (isLineComment(*MinColumnToken) ? 0 : 1);
for (SmallVectorImpl<FormatToken *>::const_iterator
     // Line comments that belong to the same line comment section are put on the
     // same line since later we might want to reflow content between them.
     // Additional fine-grained breaking of line comment sections is controlled
     // by the class BreakableLineCommentSection in case it is desirable to keep
     // several line comment sections in the same unwrapped line.
     //
     // FIXME: Consider putting separate line comment sections as children to the
     // unwrapped line instead.
-    if (isOnNewLine(**I) && JustComments && !continuesLineComment(**I, *Line))
+    (*I)->ContinuesLineCommentSection =
+        continuesLineComment(**I, *Line, CommentPragmasRegex);
+    if (isOnNewLine(**I) && JustComments && !(*I)->ContinuesLineCommentSection)
       addUnwrappedLine();
     pushToken(*I);
   }
   if (NewlineBeforeNext && JustComments)
     addUnwrappedLine();
   CommentsBeforeNextToken.clear();
 }
 
do {
 
     if (!PPStack.empty() && (PPStack.back() == PP_Unreachable) &&
         !Line->InPPDirective) {
       continue;
     }
 
     if (!FormatTok->Tok.is(tok::comment))
       return;
-    if (!continuesLineComment(*FormatTok, *Line) &&
+    FormatTok->ContinuesLineCommentSection =
+        continuesLineComment(*FormatTok, *Line, CommentPragmasRegex);
+    if (!FormatTok->ContinuesLineCommentSection &&
         (isOnNewLine(*FormatTok) || FormatTok->IsFirst)) {
       CommentsInCurrentLine = false;
     }
     if (CommentsInCurrentLine) {
       pushToken(FormatTok);
     } else {
       CommentsBeforeNextToken.push_back(FormatTok);
     }

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ReflowsComments) {
   // Don't reflow lines starting with 'XXX'.
   EXPECT_EQ("// long long long\n"
             "// long\n"
             "// XXX: long",
             format("// long long long long\n"
                    "// XXX: long",
                    getLLVMStyleWithColumns(20)));
 
+  // Don't reflow comment pragmas.
+  EXPECT_EQ("// long long long\n"
+            "// long\n"
+            "// IWYU pragma:",
+            format("// long long long long\n"
+                   "// IWYU pragma:",
+                   getLLVMStyleWithColumns(20)));
+  EXPECT_EQ("/* long long long\n"
+            " * long\n"
+            " * IWYU pragma:\n"
+            " */",
+            format("/* long long long long\n"
+                   " * IWYU pragma:\n"
+                   " */",
+                   getLLVMStyleWithColumns(20)));
+
   // Reflow lines that have a non-punctuation character among their first 2
   // characters.
   EXPECT_EQ("// long long long\n"
             "// long 'long'",
             format(
                 "// long long long long\n"
                 "// 'long'",
                 getLLVMStyleWithColumns(20)));