[clang-format] Handle CRLF correctly when formatting escaped newlines

[peterbudai]

Summary

This patch fixes clang-format lexer token handling when parsing source file with CRLF line endings.

The original bug I discovered:

#define MACRO(x) \
private:         \
  int x(int a);

was formatted correctly with LF line endings and incorrectly with CRLF line endings. In the latter case a new line was inserted before private, because it was incorrectly recognized as an identifier instead of a keyword.

The relevant code section was fixed and the affected test was updated to include CRLF versions of the test inputs as well. These test failed before and run successfully now.

[krasimir]

lib/Format/FormatTokenLexer.cpp

539

Hm, this does more. Why not just keep the DOS \r\n and the Unix \n cases, as these are the cases we're testing for? Also, please clang-format this patch.

[peterbudai]

Removed untested \r\n and \r cases, applied clang-format to the patch.

[peterbudai]

lib/Format/FormatTokenLexer.cpp

539

My idea was to make it aligned to Lexer::getCharAndSizeSlow() and Lexer::getEscapedNewLineSize() that already handled any combinations or \r and \n similarly.

But I think you're right, it's no point in adding untested cases.

[krasimir]

Looks good! Thank you!

[krasimir]

Do you need me to commit this for you?

[peterbudai]

Yes, that would be nice, thank you!

[krasimir]

• Rebase with master

lib/Format/FormatTokenLexer.cpp

FormatToken *FormatTokenLexer::getNextToken() {
   // the backslash.
   if ((Style.Language == FormatStyle::LK_JavaScript ||
        Style.Language == FormatStyle::LK_Java) &&
       FormatTok->is(tok::comment) && FormatTok->TokenText.startswith("//")) {
     size_t BackslashPos = FormatTok->TokenText.find('\\');
     while (BackslashPos != StringRef::npos) {
       if (BackslashPos + 1 < FormatTok->TokenText.size() &&
           FormatTok->TokenText[BackslashPos + 1] == '\n') {
         const char *Offset = Lex->getBufferLocation();
         Offset -= FormatTok->TokenText.size();
         Offset += BackslashPos + 1;
         resetLexer(SourceMgr.getFileOffset(Lex->getSourceLocation(Offset)));
         FormatTok->TokenText = FormatTok->TokenText.substr(0, BackslashPos + 1);
         FormatTok->ColumnWidth = encoding::columnWidthWithTabs(
             FormatTok->TokenText, FormatTok->OriginalColumn, Style.TabWidth,
             Encoding);
         break;
       }
       BackslashPos = FormatTok->TokenText.find('\\', BackslashPos + 1);
     }
   }
 
   // In case the token starts with escaped newlines, we want to
   // take them into account as whitespace - this pattern is quite frequent
   // in macro definitions.
   // FIXME: Add a more explicit test.
-  while (FormatTok->TokenText.size() > 1 && FormatTok->TokenText[0] == '\\' &&
-         FormatTok->TokenText[1] == '\n') {
+  while (FormatTok->TokenText.size() > 1 && FormatTok->TokenText[0] == '\\') {
+    unsigned SkippedWhitespace = 0;
+    if (FormatTok->TokenText.size() > 2 &&
+        (FormatTok->TokenText[1] == '\r' && FormatTok->TokenText[2] == '\n'))
+      SkippedWhitespace = 3;
+    else if (FormatTok->TokenText[1] == '\n')
+      SkippedWhitespace = 2;
+    else
+      break;
+
     ++FormatTok->NewlinesBefore;
-    WhitespaceLength += 2;
-    FormatTok->LastNewlineOffset = 2;
+    WhitespaceLength += SkippedWhitespace;
+    FormatTok->LastNewlineOffset = SkippedWhitespace;
     Column = 0;
-    FormatTok->TokenText = FormatTok->TokenText.substr(2);
+    FormatTok->TokenText = FormatTok->TokenText.substr(SkippedWhitespace);
   }
 
   FormatTok->WhitespaceRange = SourceRange(
       WhitespaceStart, WhitespaceStart.getLocWithOffset(WhitespaceLength));
 
   FormatTok->OriginalColumn = Column;
 
   TrailingWhitespace = 0;

unittests/Format/FormatTest.cpp

 TEST_F(FormatTest, FormatUnbalancedStructuralElements) {
   EXPECT_EQ("#define A \\\n  {       \\\n    {\nint i;",
             format("#define A { {\nint i;", getLLVMStyleWithColumns(11)));
   EXPECT_EQ("#define A \\\n  }       \\\n  }\nint i;",
             format("#define A } }\nint i;", getLLVMStyleWithColumns(11)));
 }
 
 TEST_F(FormatTest, EscapedNewlines) {
-  EXPECT_EQ(
-      "#define A \\\n  int i;  \\\n  int j;",
-      format("#define A \\\nint i;\\\n  int j;", getLLVMStyleWithColumns(11)));
+  FormatStyle Narrow = getLLVMStyleWithColumns(11);
+  EXPECT_EQ("#define A \\\n  int i;  \\\n  int j;",
+            format("#define A \\\nint i;\\\n  int j;", Narrow));
   EXPECT_EQ("#define A\n\nint i;", format("#define A \\\n\n int i;"));
   EXPECT_EQ("template <class T> f();", format("\\\ntemplate <class T> f();"));
   EXPECT_EQ("/* \\  \\  \\\n */", format("\\\n/* \\  \\  \\\n */"));
   EXPECT_EQ("<a\n\\\\\n>", format("<a\n\\\\\n>"));
 
+  FormatStyle AlignLeft = getLLVMStyle();
+  AlignLeft.AlignEscapedNewlines = FormatStyle::ENAS_Left;
+  EXPECT_EQ("#define MACRO(x) \\\n"
+            "private:         \\\n"
+            "  int x(int a);\n",
+            format("#define MACRO(x) \\\n"
+                   "private:         \\\n"
+                   "  int x(int a);\n",
+                   AlignLeft));
+
+  // CRLF line endings
+  EXPECT_EQ("#define A \\\r\n  int i;  \\\r\n  int j;",
+            format("#define A \\\r\nint i;\\\r\n  int j;", Narrow));
+  EXPECT_EQ("#define A\r\n\r\nint i;", format("#define A \\\r\n\r\n int i;"));
+  EXPECT_EQ("template <class T> f();", format("\\\ntemplate <class T> f();"));
+  EXPECT_EQ("/* \\  \\  \\\r\n */", format("\\\r\n/* \\  \\  \\\r\n */"));
+  EXPECT_EQ("<a\r\n\\\\\r\n>", format("<a\r\n\\\\\r\n>"));
+  EXPECT_EQ("#define MACRO(x) \\\r\n"
+            "private:         \\\r\n"
+            "  int x(int a);\r\n",
+            format("#define MACRO(x) \\\r\n"
+                   "private:         \\\r\n"
+                   "  int x(int a);\r\n",
+                   AlignLeft));
+
   FormatStyle DontAlign = getLLVMStyle();
   DontAlign.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
   DontAlign.MaxEmptyLinesToKeep = 3;
   // FIXME: can't use verifyFormat here because the newline before
   // "public:" is not inserted the first time it's reformatted
   EXPECT_EQ("#define A \\\n"
             "  class Foo { \\\n"
             "    void bar(); \\\n"
             "\\\n"
             "\\\n"
             "\\\n"
             "  public: \\\n"
             "    void baz(); \\\n"
             "  };",
             format("#define A \\\n"
                    "  class Foo { \\\n"
                    "    void bar(); \\\n"
                    "\\\n"
                    "\\\n"
                    "\\\n"
                    "  public: \\\n"
                    "    void baz(); \\\n"
-                   "  };", DontAlign));
+                   "  };",
+                   DontAlign));
 }
 
 TEST_F(FormatTest, CalculateSpaceOnConsecutiveLinesInMacro) {
   verifyFormat("#define A \\\n"
                "  int v(  \\\n"
                "      a); \\\n"
                "  int i;",
                getLLVMStyleWithColumns(11));