[clang-tidy][modernize-raw-string-literal] Don't replace upper ASCII with raw literals

[zinovy.nis]

Summary

It's useless and not safe to replace UTF-8 encoded with escaped ASCII to raw UTF-8 chars:

"\xE2\x98\x83" ---> "☃" (snowman)

Especially there's an explicit test for ASCII in this check.

[zinovy.nis]

test/clang-tidy/modernize-raw-string-literal.cpp

42–44

By the way, AFAIK the lines above are not checked for fixes (and fixes absence) at all! Is it correct?

[aaron.ballman]

clang-tidy/modernize/RawStringLiteralCheck.cpp

70–72

I think you can use isASCII() from CharInfo.h rather than reimplement it.

[LegalizeAdulthood]

test/clang-tidy/modernize-raw-string-literal.cpp

44

IIRC, the default behavior is that if no matching CHECK-FIXES line is found, then it is considered a failure. Have you tried your test code without your change to verify that this is the case?

[zinovy.nis]

clang-tidy/modernize/RawStringLiteralCheck.cpp

70–72

Nice finding! Thanks!

test/clang-tidy/modernize-raw-string-literal.cpp

44

1. Without my fix my test fails with a Python decoder error as it cannot print Unicode symbols in Windows console. But no FileCheck errors occur:

UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 974: ordinal not in range(128)

2. Regardless of my changes when I replace "char const *const Delete("\\\177");" with "char const *const Delete("\000\\\177");" (leading \0) test still passes! Looks like CHECK-FIXes must be explicit.

[zinovy.nis]

• Use clang::isASCII instead of home-brewed code.

[aaron.ballman]

clang-tidy/modernize/RawStringLiteralCheck.cpp

71

I am starting to think that this functionality should be refactored because the check is now O(N^2) in the worst case because all of the bytes of the string need to be touched twice. It would be nice for performance reasons to combine this so that there's only a single pass over all of the characters.

What do you think?

[zinovy.nis]

clang-tidy/modernize/RawStringLiteralCheck.cpp

71

Sorry, but why O(N^2)? isASCII is O(1), it's just return C<=127. find_if_not is O(N).

[aaron.ballman]

clang-tidy/modernize/RawStringLiteralCheck.cpp

71

The find_if_not() call you add touches every character in the string to see if it's ASCII and it all characters are ASCII, then containsEscape() calls find() which touches every character again (assuming the searched character is never encountered).

Looking a bit deeper, isRawStringLiteral() also calls find(), but it asserts that the character is found, so not every character is touched in the string and it should find a the searched character quite quickly.

[zinovy.nis]

clang-tidy/modernize/RawStringLiteralCheck.cpp

71

OK, I'll see how to combine theses checks into a single one.

But anyway I see only 2*O(N), not O(N^2) here.

[aaron.ballman]

clang-tidy/modernize/RawStringLiteralCheck.cpp

71

Oh, derp, that's my thinko -- sorry! You are correct, that's 2 * O(N) and not O(N^2).

[zinovy.nis]

clang-tidy/modernize/RawStringLiteralCheck.cpp

71

May be we can use here std::isprint

[zinovy.nis]

• Optimized containsEscapedCharacters not to re-create bitset<char> (implicitly in StringRef::find_first_of) for each literal.
• Merged 2 passes for testing for allowed chars into a single one.

[zinovy.nis]

Aaron, any comments for the new revision?

[zinovy.nis]

Gentle ping.

[LegalizeAdulthood]

Other than a few minor nits, ship it.

clang-tidy/modernize/RawStringLiteralCheck.cpp

110

"Upper ASCII" is a misnomer. ASCII only defines character codes 0-127. Non-ASCII codes are what is being described here.

111

Why does this loop go down instead of up? I would have expected a more conventional

for (unsigned char C = 0x80u; C <= 0xFFu; ++C)

clang-tidy/modernize/RawStringLiteralCheck.h

39 ↗

(On Diff #144066)

Consider introducing a typedef for this ugly type name.

test/clang-tidy/modernize-raw-string-literal.cpp

44

Consider adding CHECK-FIXES lines for the other test cases that should remain unmolested by the check.

[zinovy.nis]

clang-tidy/modernize/RawStringLiteralCheck.cpp

111

The only reason is that ++C for C==0xFFu is 0 so C <= 0xFF is always satisfied leading to inifinite loop.

[LegalizeAdulthood]

clang-tidy/modernize/RawStringLiteralCheck.cpp

111

OK, makes sense. I rarely write loops that iterate over character types.

[zinovy.nis]

test/clang-tidy/modernize-raw-string-literal.cpp

44

I'm going to make a separate commit for that, OK?

[zinovy.nis]

Fixed in git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@331297 91177308-0d34-0410-b5e6-96231b3b80d8

[zinovy.nis]

https://reviews.llvm.org/rCTE331297

[alexfh]

test/clang-tidy/modernize-raw-string-literal.cpp

42–44

These test cases without corresponding CHECK-FIXES/CHECK-MESSAGES lines will work (that is ensure no fixes are applied to the code) under the assumption that all warnings explicitly covered by CHECK-MESSAGE lines will only contain fixes for relevant code (in most cases in the same line), and there are CHECK-FIXES for all of them.

And if there is no CHECK-MESSAGES line then FileCheck -implicit-check-not='warning|error' -check-prefix=CHECK-MESSAGES (used in the check_clang_tidy.py) will fail if there is an unaccounted (i.e. not covered by a CHECK-MESSAGES: pattern) warning.

The assumption won't work if the check makes distant fixes (for example, if it inserts #include directives, there should be a CHECK-FIXES line for that).

Thus there's not much value in checking these lines are left unmodified. But in case there is a warning which should contain no fix, checking that the code was not modified by clang-tidy is quite useful.

We could make this more explicit in different ways:

1. Run FileCheck on the result of diff original-file file-processed-by-clang-tidy. That may make tests much more verbose.
2. Run FileCheck on the result of clang-tidy -export-fixes. That would make the tests both more verbose and harder to verify for sanity (applying textual replacements with given byte offsets is not what a human brain does well).

Better ideas are welcome.

clang-tidy/modernize/RawStringLiteralCheck.cpp

 #include "RawStringLiteralCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/CharInfo.h"
 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;

                                     33)) != StringRef::npos)
     return false;
 
+  // Don't replace chars from upper ASCII as they can represent UTF-8 or be
+  // supported by specific code pages only.
+  if (Bytes.find_if_not(isASCII) != StringRef::npos)
+    return false;
+
   CharSourceRange CharRange = Lexer::makeFileCharRange(
       CharSourceRange::getTokenRange(Literal->getSourceRange()),
       *Result.SourceManager, Result.Context->getLangOpts());

test/clang-tidy/modernize-raw-string-literal.cpp

 char const *const Us("goink\\\037");
 char const *const HexNonPrintable("\\\x03");
 char const *const Delete("\\\177");
+char const *const MultibyteSnowman("\xE2\x98\x83");
+// CHECK-FIXES: {{^}}char const *const MultibyteSnowman("\xE2\x98\x83");{{$}}
 
 char const *const TrailingSpace("A line \\with space. \n");
 char const *const TrailingNewLine("A single \\line.\n");