[clang-tidy] Don't modernize-raw-string-literal if replacement is longer.

[leanil]

Summary

modernize-raw-string-literal suggests to replace e.g.:

std::string s{ "foo \"" } with std::string s{R"(foo ")" };
std::string s{ ":)\"" }   with std::string s{R"lit(:)")lit" };

which are not necessarily improvements.
Maybe it should only suggest replacements, that are not longer than the original.

Fixes PR30964

[alexfh]

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

98

Does this test fail without the patch? Also, should there be a test covering the case where no replacement is made (and the current code issues a warning)?

[leanil]

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

98

No, it doesn't. This is to check that MeasureTokenLength works as I expect. It wouldn't warn if it measured the two parts separately.
And I left the check here for those who may have the same concerns, although it's indeed not the core functionality I test here. Should I remove it?

[alexfh]

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

98

The test is fine, but it's better to add it as a separate patch. This patch needs proper tests of the new functionality.

[leanil]

Add test cases for the new functionality.
These should not produce warnings, because their raw string replacement would be longer.

[LegalizeAdulthood]

Don't do this without introducing an option to turn it off.

[LegalizeAdulthood]

In D28667#649626, @LegalizeAdulthood wrote:

Don't do this without introducing an option to turn it off.

To clarify my reasoning:
You imposing your subjective idea of goodness on the code, i.e. style. Other people will have different ideas about what they want from the code. So, to quote the X Window System Maxim, provide tools not rules. Allow people to say whether or not they want the raw string literal in this case.

[leanil]

In D28667#650585, @LegalizeAdulthood wrote:

In D28667#649626, @LegalizeAdulthood wrote:

Don't do this without introducing an option to turn it off.

To clarify my reasoning:
You imposing your subjective idea of goodness on the code, i.e. style. Other people will have different ideas about what they want from the code. So, to quote the X Window System Maxim, provide tools not rules. Allow people to say whether or not they want the raw string literal in this case.

I agree. Which mode do you think should be the default?

[LegalizeAdulthood]

In D28667#650586, @leanil wrote:

In D28667#650585, @LegalizeAdulthood wrote:

In D28667#649626, @LegalizeAdulthood wrote:

Don't do this without introducing an option to turn it off.

To clarify my reasoning:
You imposing your subjective idea of goodness on the code, i.e. style. Other people will have different ideas about what they want from the code. So, to quote the X Window System Maxim, provide tools not rules. Allow people to say whether or not they want the raw string literal in this case.

I agree. Which mode do you think should be the default?

As long as you can control it, I don't think it matters much and preference of the submitter is fine.

[leanil]

Add config parameter to control new functionality.
Update the test cases accordingly.

[alexfh]

LG

clang-tidy/modernize/RawStringLiteralCheck.h

public:
 
   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);
+      const StringLiteral *Literal, StringRef Replacement);
 
   std::string DelimiterStem;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_RAW_STRING_LITERAL_H

clang-tidy/modernize/RawStringLiteralCheck.cpp

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))
-    replaceWithRawStringLiteral(Result, Literal);
+  if (containsEscapedCharacters(Result, Literal)) {
+    std::string Replacement = asRawStringLiteral(Literal, DelimiterStem);
+    if (Replacement.length() <=
+        Lexer::MeasureTokenLength(Literal->getLocStart(), *Result.SourceManager,
+                                  getLangOpts()))
+      replaceWithRawStringLiteral(Result, Literal, Replacement);
+  }
 }
 
 void RawStringLiteralCheck::replaceWithRawStringLiteral(
-    const MatchFinder::MatchResult &Result, const StringLiteral *Literal) {
+    const MatchFinder::MatchResult &Result, const StringLiteral *Literal,
+    StringRef Replacement) {
   CharSourceRange CharRange = Lexer::makeFileCharRange(
       CharSourceRange::getTokenRange(Literal->getSourceRange()),
       *Result.SourceManager, getLangOpts());
   diag(Literal->getLocStart(),
        "escaped string literal can be written as a raw string literal")
-      << FixItHint::CreateReplacement(
-             CharRange, asRawStringLiteral(Literal, DelimiterStem));
+      << FixItHint::CreateReplacement(CharRange, Replacement);
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

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

 // support char16_t and char32_t (VS2013 does not)
 // char16_t const *const UTF16Literal(u"foobie\\bletch");
 // char16_t const *const UTF16RawLiteral(uR"(foobie\\bletch)");
 // char32_t const *const UTF32Literal(U"foobie\\bletch");
 // char32_t const *const UTF32RawLiteral(UR"(foobie\\bletch)");
 wchar_t const *const WideLiteral(L"foobie\\bletch");
 wchar_t const *const WideRawLiteral(LR"(foobie\\bletch)");
 
+// Don't replace these, because the raw literal would be longer.
+char const *const JustAQuote("quote:\'");
+char const *const NeedDelimiter("\":)\"");
+
 char const *const SingleQuote("goink\'frob");
 // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
 // CHECK-XFIXES: {{^}}char const *const SingleQuote(R"(goink'frob)");{{$}}
 
 char const *const DoubleQuote("goink\"frob");
 // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
 // CHECK-FIXES: {{^}}char const *const DoubleQuote(R"(goink"frob)");{{$}}
 

 char const *const OctalPrintable("\100\\");
 // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: {{.*}} can be written as a raw string literal
 // CHECK-FIXES: {{^}}char const *const OctalPrintable(R"(@\)");{{$}}
 
 char const *const HexPrintable("\x40\\");
 // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: {{.*}} can be written as a raw string literal
 // CHECK-FIXES: {{^}}char const *const HexPrintable(R"(@\)");{{$}}
 
 char const *const prettyFunction(__PRETTY_FUNCTION__);
 char const *const function(__FUNCTION__);
 char const *const func(__func__);
 
 #define TRICK(arg_) #arg_
 char const *const MacroBody = TRICK(foo\\bar);
 
 #define HAT(rabbit_) #rabbit_ "foo\\bar"
 char const *const StringizedMacroArgument = HAT(foo\\bar);