[PATCH] D30748: [Lexer] Finding beginning of token with escaped new line

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Sorry for the delay, I was on vacation.

This looks much better now, thanks! A few more comments though.



================
Comment at: lib/Lex/Lexer.cpp:460
+/// \brief Check if new line pointed by Str is escaped.
+bool isNewLineEscaped(const char *BufferStart, const char *Str) {
+  assert(isVerticalWhitespace(Str[0]));
----------------
The way the function is exposed to the test may lead to confusion. I'd either properly declare it in the header (and place it in a namespace, if it is not yet) or at least leave a comment here that the function is not static, since it needs to be exposed to the test.


================
Comment at: lib/Lex/Lexer.cpp:474
+  // Rewind to first non-space character:
+  while (isHorizontalWhitespace(*Str) && Str > BufferStart)
+    --Str;
----------------
nit: Placing the `Str > BufferStart` first would make it more obvious that `Str` can be safely dereferenced.


================
Comment at: unittests/Lex/LexerTest.cpp:371-386
+  std::vector<std::pair<bool, std::string>> TestLines = {
+      {true, "\\\r"},    {true, "\\\n"},        {true, "\\\r\n"},
+      {true, "\\\n\r"},  {true, "\\ \t\v\f\r"}, {true, "\\ \t\v\f\r\n"},
+      {false, "\\\r\r"}, {false, "\\\r\r\n"},   {false, "\\\n\n"},
+      {false, "\r"},     {false, "\n"},         {false, "\r\n"},
+      {false, "\n\r"},   {false, "\r\r"},       {false, "\n\n"}};
+
----------------
I would better unroll the loop:

  auto endsWithEscapedNewline = [] (const char *S) {
    return isNewLineEscaped(S, S + strlen(S) - 1);
  };
  EXPECT_TRUE(endsWithEscapedNewline("\\\r"));
  EXPECT_TRUE(endsWithEscapedNewline("\\\n"));
  ...
  EXPECT_FALSE(endsWithEscapedNewline("\\\r\r"));
  ...

This would simplify the test and make EXPECT_* macro output sufficient to detect failing patterns without any clarifying messages.


https://reviews.llvm.org/D30748