[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

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

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

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.h

 //===--- RawStringLiteralCheck.h - clang-tidy--------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_RAW_STRING_LITERAL_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_RAW_STRING_LITERAL_H
 
 #include "../ClangTidy.h"
+#include <bitset>
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
 /// This check replaces string literals with escaped characters to
 /// raw string literals.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/modernize-raw-string-literal.html
 class RawStringLiteralCheck : public ClangTidyCheck {
 public:
   RawStringLiteralCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Options) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   void replaceWithRawStringLiteral(
       const ast_matchers::MatchFinder::MatchResult &Result,
       const StringLiteral *Literal, StringRef Replacement);
 
   std::string DelimiterStem;
+  std::bitset<1 << CHAR_BIT> DisallowedChars;
   const bool ReplaceShorterLiterals;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_RAW_STRING_LITERAL_H

clang-tidy/modernize/RawStringLiteralCheck.cpp

 
 bool isRawStringLiteral(StringRef Text) {
   // Already a raw string literal if R comes before ".
   const size_t QuotePos = Text.find('"');
   assert(QuotePos != StringRef::npos);
   return (QuotePos > 0) && (Text[QuotePos - 1] == 'R');
 }
 
-bool containsEscapedCharacters(const MatchFinder::MatchResult &Result,
-                               const StringLiteral *Literal) {
+bool containsEscapedCharacters(
+    const MatchFinder::MatchResult &Result, const StringLiteral *Literal,
+    const std::bitset<1 << CHAR_BIT> &DisallowedChars) {
   // FIXME: Handle L"", u8"", u"" and U"" literals.
   if (!Literal->isAscii())
     return false;
 
-  StringRef Bytes = Literal->getBytes();
-  // Non-printing characters disqualify this literal:
-  // \007 = \a bell
-  // \010 = \b backspace
-  // \011 = \t horizontal tab
-  // \012 = \n new line
-  // \013 = \v vertical tab
-  // \014 = \f form feed
-  // \015 = \r carriage return
-  // \177 = delete
-  if (Bytes.find_first_of(StringRef("\000\001\002\003\004\005\006\a"
-                                    "\b\t\n\v\f\r\016\017"
-                                    "\020\021\022\023\024\025\026\027"
-                                    "\030\031\032\033\034\035\036\037"
-                                    "\177",
-                                    33)) != StringRef::npos)
+  for (const unsigned char C : Literal->getBytes())
+    if (DisallowedChars.test(C))
       return false;
 
   CharSourceRange CharRange = Lexer::makeFileCharRange(
       CharSourceRange::getTokenRange(Literal->getSourceRange()),
       *Result.SourceManager, Result.Context->getLangOpts());
   StringRef Text = Lexer::getSourceText(CharRange, *Result.SourceManager,
                                         Result.Context->getLangOpts());
   if (isRawStringLiteral(Text))
     return false;
 
   return containsEscapes(Text, R"('\"?x01)");
 }
 
 bool containsDelimiter(StringRef Bytes, const std::string &Delimiter) {
   return Bytes.find(Delimiter.empty()
                         ? std::string(R"lit()")lit")
                         : (")" + Delimiter + R"(")")) != StringRef::npos;
 }
 
 std::string asRawStringLiteral(const StringLiteral *Literal,
                                const std::string &DelimiterStem) {
   const StringRef Bytes = Literal->getBytes();
   std::string Delimiter;
   for (int I = 0; containsDelimiter(Bytes, Delimiter); ++I) {
     Delimiter = (I == 0) ? DelimiterStem : DelimiterStem + std::to_string(I);
   }
 
   if (Delimiter.empty())
     return (R"(R"()" + Bytes + R"lit()")lit").str();
 
   return (R"(R")" + Delimiter + "(" + Bytes + ")" + Delimiter + R"(")").str();
 }
 
 } // namespace
 
 RawStringLiteralCheck::RawStringLiteralCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       DelimiterStem(Options.get("DelimiterStem", "lit")),
-      ReplaceShorterLiterals(Options.get("ReplaceShorterLiterals", false)) {}
+      ReplaceShorterLiterals(Options.get("ReplaceShorterLiterals", false)) {
+  // Non-printing characters are disallowed:
+  // \007 = \a bell
+  // \010 = \b backspace
+  // \011 = \t horizontal tab
+  // \012 = \n new line
+  // \013 = \v vertical tab
+  // \014 = \f form feed
+  // \015 = \r carriage return
+  // \177 = delete
+  for (const unsigned char C : StringRef("\000\001\002\003\004\005\006\a"
+                                         "\b\t\n\v\f\r\016\017"
+                                         "\020\021\022\023\024\025\026\027"
+                                         "\030\031\032\033\034\035\036\037"
+                                         "\177",
+                                         33))
+    DisallowedChars.set(C);
+
+  // Upper ASCII are disallowed too.
+  for (unsigned char C = 0xFFu; C >= 0x80u; --C)
+    DisallowedChars.set(C);
+}
 
 void RawStringLiteralCheck::storeOptions(ClangTidyOptions::OptionMap &Options) {
   ClangTidyCheck::storeOptions(Options);
   this->Options.store(Options, "ReplaceShorterLiterals",
                       ReplaceShorterLiterals);
 }
 
 void RawStringLiteralCheck::registerMatchers(MatchFinder *Finder) {
   // Raw string literals require C++11 or later.
   if (!getLangOpts().CPlusPlus11)
     return;
 
   Finder->addMatcher(
       stringLiteral(unless(hasParent(predefinedExpr()))).bind("lit"), this);
 }
 
 void RawStringLiteralCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Literal = Result.Nodes.getNodeAs<StringLiteral>("lit");
   if (Literal->getLocStart().isMacroID())
     return;
 
-  if (containsEscapedCharacters(Result, Literal)) {
+  if (containsEscapedCharacters(Result, Literal, DisallowedChars)) {
     std::string Replacement = asRawStringLiteral(Literal, DelimiterStem);
     if (ReplaceShorterLiterals ||
         Replacement.length() <=
             Lexer::MeasureTokenLength(Literal->getLocStart(),
                                       *Result.SourceManager, getLangOpts()))
       replaceWithRawStringLiteral(Result, Literal, Replacement);
   }
 }

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

 char const *const Em("goink\\\031");
 char const *const Sub("goink\\\032");
 char const *const Esc("goink\\\033");
 char const *const Fs("goink\\\034");
 char const *const Gs("goink\\\035");
 char const *const Rs("goink\\\036");
 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");
 char const *const AlreadyRaw(R"(foobie\\bletch)");
 char const *const UTF8Literal(u8"foobie\\bletch");
 char const *const UTF8RawLiteral(u8R"(foobie\\bletch)");
 // TODO: enable these tests once all supported compilers
 // support char16_t and char32_t (VS2013 does not)