clang-format: do not reflow bullet lists

[Typz]

Summary

This patch prevents reflowing bullet lists in block comments.

It handles all lists supported by doxygen and markdown, e.g. bullet
lists starting with '-', '*', '+', as well as numbered lists starting
with -# or a number followed by a dot.

[krasimir]

lib/Format/BreakableToken.cpp

313

A problem with this is that sometimes you have a sentence ending with a number, like this one, in 2016. If this sentence also happens to just go over the column width, its last part would be reflown and during subsequent passes it will be seen as a numbered list, which is super unfortunate. I'd like us to come up with a more refined strategy of handling this case. Maybe we should look at how others are doing it?

315

This builds an llvm::Regex on each invocation, which is wasteful.

unittests/Format/FormatTestComments.cpp

1662

I'd also like to see tests where we correctly reflow lists with multiline entries.

[Typz]

lib/Format/BreakableToken.cpp

313

Looking at doxygen, it seems there is no extra protection: just a number followed by a dot...
So it means:

• We should never break before a such a sequence, to avoid the issue.
• We may also limit the expression to limit the size of the number: I am not sure there are cases where bullet lists with hundreds of items are used, esp. with explicit values (uses the auto-numbering -# would be much simpler in that case). Maybe a limit of 2 digits? The same limit would be applied to prevent breaking before a number followed by a dot.

What do you think?

315

I did this to keep the function re-entrant; but since the code is not multi-thread I can use a static variable instead.

[Typz]

• Use static regex to avoid recreating it each time
• Add more tests

[krasimir]

lib/Format/BreakableToken.cpp

313

I like the combination of the two options: let's limit to 2 digits and not break before a matching numbered list sequence followed by a fullstop. That would require also a little change to BreakableToken::getCommentSplit.

[Typz]

lib/Format/BreakableToken.cpp

313

Done, but I could find a use-case where this would break subsequent passes, apart from re-running clang-format ; but in this case it is fine, since the comments are already formatted to fit, and will thus not be reflowed...

[Typz]

Limit to 2 digits and not break before a matching numbered list sequence followed by a fullstop, to avoid interpreting numbers at the end of sentence as numbered bullets (and thus preventing reflow).

[krasimir]

Looks great!

lib/Format/BreakableToken.cpp

 
 const FormatToken &BreakableComment::tokenAt(unsigned LineIndex) const {
   return Tokens[LineIndex] ? *Tokens[LineIndex] : Tok;
 }
 
 static bool mayReflowContent(StringRef Content) {
   Content = Content.trim(Blanks);
   // Lines starting with '@' commonly have special meaning.
-  static const SmallVector<StringRef, 4> kSpecialMeaningPrefixes = {
-      "@", "TODO", "FIXME", "XXX"};
+  // Lines starting with '-', '-#', '+' or '*' are bulleted/numbered lists.
+  static const SmallVector<StringRef, 8> kSpecialMeaningPrefixes = {
+      "@", "TODO", "FIXME", "XXX", "-# ", "- ", "+ ", "* " };
   bool hasSpecialMeaningPrefix = false;
   for (StringRef Prefix : kSpecialMeaningPrefixes) {
     if (Content.startswith(Prefix)) {
       hasSpecialMeaningPrefix = true;
       break;
     }
   }
+
+  // Numbered lists may also start with a number followed by '.'
+  static llvm::Regex kNumberedListRegexp = llvm::Regex("^[1-9][0-9]*\\. ");
+  hasSpecialMeaningPrefix = hasSpecialMeaningPrefix ||
+                            kNumberedListRegexp.match(Content);
+
   // Simple heuristic for what to reflow: content should contain at least two
   // characters and either the first or second character must be
   // 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]));

unittests/Format/FormatTestComments.cpp

TEST_F(FormatTestComments, ReflowsComments) {
   EXPECT_EQ("/* long long long\n"
             " * long\n"
             " *\n"
             " * long */",
             format("/* long long long long\n"
                    " *\n"
                    " * long */",
                    getLLVMStyleWithColumns(20)));
-  
   // Don't reflow lines having content that is a single character.
   EXPECT_EQ("// long long long\n"
             "// long\n"
             "// l",
             format("// long long long long\n"
                    "// l",
                    getLLVMStyleWithColumns(20)));
 
   // Don't reflow lines starting with two punctuation characters.
   EXPECT_EQ("// long long long\n"
             "// long\n"
             "// ... --- ...",
             format(
                 "// long long long long\n"
                 "// ... --- ...",
                 getLLVMStyleWithColumns(20)));
 
   // Don't reflow lines starting with '@'.
   EXPECT_EQ("// long long long\n"
             "// long\n"
             "// @param arg",
             format("// long long long long\n"
                    "// @param arg",
                    getLLVMStyleWithColumns(20)));
-  
   // Don't reflow lines starting with 'TODO'.
   EXPECT_EQ("// long long long\n"
             "// long\n"
             "// TODO: long",
             format("// long long long long\n"
                    "// TODO: long",
                    getLLVMStyleWithColumns(20)));
 
TEST_F(FormatTestComments, ReflowsComments) {
   // Don't reflow lines having different indentation.
   EXPECT_EQ("// long long long\n"
             "// long\n"
             "//  long",
             format("// long long long long\n"
                    "//  long",
                    getLLVMStyleWithColumns(20)));
 
+  // Don't reflow separate bullets in list
+  EXPECT_EQ("// - long long long\n"
+            "// long\n"
+            "// - long",
+            format("// - long long long long\n"
+                   "// - long",
+                   getLLVMStyleWithColumns(20)));
+  EXPECT_EQ("// * long long long\n"
+            "// long\n"
+            "// * long",
+            format("// * long long long long\n"
+                   "// * long",
+                   getLLVMStyleWithColumns(20)));
+  EXPECT_EQ("// + long long long\n"
+            "// long\n"
+            "// + long",
+            format("// + long long long long\n"
+                   "// + long",
+                   getLLVMStyleWithColumns(20)));
+  EXPECT_EQ("// 1. long long long\n"
+            "// long\n"
+            "// 2. long",
+            format("// 1. long long long long\n"
+                   "// 2. long",
+                   getLLVMStyleWithColumns(20)));
+  EXPECT_EQ("// -# long long long\n"
+            "// long\n"
+            "// -# long",
+            format("// -# long long long long\n"
+                   "// -# long",
+                   getLLVMStyleWithColumns(20)));
+
+  EXPECT_EQ("// - long long long\n"
+            "// long long long\n"
+            "// - long",
+            format("// - long long long long\n"
+                   "// long long\n"
+                   "// - long",
+                   getLLVMStyleWithColumns(20)));
+  EXPECT_EQ("// - long long long\n"
+            "// long long long\n"
+            "// long\n"
+            "// - long",
+            format("// - long long long long\n"
+                   "// long long long\n"
+                   "// - long",
+                   getLLVMStyleWithColumns(20)));
+
   // Don't break or reflow after implicit string literals.
   verifyFormat("#include <t> // l l l\n"
                "             // l",
                getLLVMStyleWithColumns(20));
 
   // Don't break or reflow comments on import lines.
   EXPECT_EQ("#include \"t\" /* l l l\n"
             "                * l */",