[clang-format] Remove code related to trigraphs #148640
Conversation
When reviewing llvm#147156, the reviewers pointed out that we don't need to support the trigraph. The code never handled it right. In the debug build, this kind of input caused the assertion in the function `countLeadingWhitespace` to fail. The release build without assertions outputted `?` `?` `/` separated by spaces. ```C #define A ??/ int i; ``` This is because the code in `countLeadingWhitespace` assumed that the underlying lexer recognized the entire `??/` sequence as a single token. In fact, the lexer recognized it as 3 separate tokens. The flag to make the lexer recognize trigraphs was never enabled. This patch enables the flag in the underlying lexer. This way, the program now either turns the trigraph into a single `\` or removes it altogether if the line is short enough. There are operators like the `??=` in C#. So the flag is not enabled for all input languages. Instead the check for the token size is moved from the assert line into the if line. The problem was introduced by my own patch 370bee4 from about 3 years ago. I added code to count the number of characters in the escape sequence probably just because the block of code used to have a comment saying someone should add the feature. Maybe I forgot to enable assertions when I ran the code. I found the problem because reviewing pull request 145243 made me look at the code again.
|
@llvm/pr-subscribers-clang-format Author: None (sstwcw) ChangesWhen reviewing #147156, the reviewers pointed out that we didn't need to support the trigraph. The code never handled it right. In the debug build, this kind of input caused the assertion in the function #define A ??/
int i;This is because the code in This patch enables the flag in the underlying lexer. This way, the program now either turns the trigraph into a single The problem was introduced by my own patch 370bee4 from about 3 years ago. I added code to count the number of characters in the escape sequence probably just because the block of code used to have a comment saying someone should add the feature. Maybe I forgot to enable assertions when I ran the code. I found the problem because reviewing pull request 145243 made me look at the code again. Full diff: https://github.com/llvm/llvm-project/pull/148640.diff 2 Files Affected:
diff --git a/clang/lib/Format/FormatTokenLexer.cpp b/clang/lib/Format/FormatTokenLexer.cpp
index 49da3160daf50..ea51f88f2d975 100644
--- a/clang/lib/Format/FormatTokenLexer.cpp
+++ b/clang/lib/Format/FormatTokenLexer.cpp
@@ -1198,7 +1198,7 @@ void FormatTokenLexer::truncateToken(size_t NewLen) {
/// Count the length of leading whitespace in a token.
static size_t countLeadingWhitespace(StringRef Text) {
// Basically counting the length matched by this regex.
- // "^([\n\r\f\v \t]|(\\\\|\\?\\?/)[\n\r])+"
+ // "^([\n\r\f\v \t]|\\\\[\n\r])+"
// Directly using the regex turned out to be slow. With the regex
// version formatting all files in this directory took about 1.25
// seconds. This version took about 0.5 seconds.
@@ -1222,13 +1222,6 @@ static size_t countLeadingWhitespace(StringRef Text) {
break;
// Splice found, consume it.
Cur = Lookahead + 1;
- } else if (Cur[0] == '?' && Cur[1] == '?' && Cur[2] == '/' &&
- (Cur[3] == '\n' || Cur[3] == '\r')) {
- // Newlines can also be escaped by a '?' '?' '/' trigraph. By the way, the
- // characters are quoted individually in this comment because if we write
- // them together some compilers warn that we have a trigraph in the code.
- assert(End - Cur >= 4);
- Cur += 4;
} else {
break;
}
@@ -1300,22 +1293,16 @@ FormatToken *FormatTokenLexer::getNextToken() {
Style.TabWidth - (Style.TabWidth ? Column % Style.TabWidth : 0);
break;
case '\\':
- case '?':
- case '/':
- // The text was entirely whitespace when this loop was entered. Thus
- // this has to be an escape sequence.
- assert(Text.substr(i, 4) == "\?\?/\r" ||
- Text.substr(i, 4) == "\?\?/\n" ||
- (i >= 1 && (Text.substr(i - 1, 4) == "\?\?/\r" ||
- Text.substr(i - 1, 4) == "\?\?/\n")) ||
- (i >= 2 && (Text.substr(i - 2, 4) == "\?\?/\r" ||
- Text.substr(i - 2, 4) == "\?\?/\n")) ||
- (Text[i] == '\\' && [&]() -> bool {
- size_t j = i + 1;
- while (j < Text.size() && isHorizontalWhitespace(Text[j]))
- ++j;
- return j < Text.size() && (Text[j] == '\n' || Text[j] == '\r');
- }()));
+ // The code preceding the loop and in the countLeadingWhitespace
+ // function guarantees that Text is entirely whitespace, not including
+ // comments but including escaped newlines. So if 1 of these characters
+ // show up, then it has to be in an escape sequence.
+ assert(Text[i] == '\\' && [&]() -> bool {
+ size_t j = i + 1;
+ while (j < Text.size() && isHorizontalWhitespace(Text[j]))
+ ++j;
+ return j < Text.size() && (Text[j] == '\n' || Text[j] == '\r');
+ }());
InEscape = true;
break;
default:
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 0bc1c6d45656e..46e55da5b5e11 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -6681,6 +6681,17 @@ TEST_F(FormatTest, EscapedNewlines) {
" int x(int a);",
AlignLeft);
+ // Escaped with a trigraph. The program just has to avoid crashing.
+ verifyNoCrash("#define A \?\?/\n"
+ "int i;\?\?/\n"
+ " int j;");
+ verifyNoCrash("#define A \?\?/\r\n"
+ "int i;\?\?/\r\n"
+ " int j;");
+ verifyNoCrash("#define A \?\?/\n"
+ "int i;",
+ getGoogleStyle(FormatStyle::LK_CSharp));
+
// CRLF line endings
verifyFormat("#define A \\\r\n int i; \\\r\n int j;",
"#define A \\\r\nint i;\\\r\n int j;", Narrow);
|
There was a problem hiding this comment.
Hi, I've requested the reviewers of #147156 for this since I don't understand this topic all too well.
| // function guarantees that Text is entirely whitespace, not including | ||
| // comments but including escaped newlines. So if 1 of these characters | ||
| // show up, then it has to be in an escape sequence. | ||
| assert(Text[i] == '\\' && [&]() -> bool { |
There was a problem hiding this comment.
| assert(Text[i] == '\\' && [&]() -> bool { | |
| assert([&]() -> bool { |
When reviewing #147156, the reviewers pointed out that we didn't need to support the trigraph. The code never handled it right.
In the debug build, this kind of input caused the assertion in the function
countLeadingWhitespaceto fail. The release build without assertions outputted??/separated by spaces.This is because the code in
countLeadingWhitespaceassumed that the underlying lexer recognized the entire??/sequence as a single token. In fact, the lexer recognized it as 3 separate tokens. The flag to make the lexer recognize trigraphs was never enabled.This patch enables the flag in the underlying lexer. This way, the program now either turns the trigraph into a single
\or removes it altogether if the line is short enough. There are operators like the??=in C#. So the flag is not enabled for all input languages. Instead the check for the token size is moved from the assert line into the if line.The problem was introduced by my own patch 370bee4 from about 3 years ago. I added code to count the number of characters in the escape sequence probably just because the block of code used to have a comment saying someone should add the feature. Maybe I forgot to enable assertions when I ran the code. I found the problem because reviewing pull request 145243 made me look at the code again.