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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
