[clang-tidy] Detect string literals in macros in modernize-raw-string… #133636

carlosgalvezp · 2025-03-30T12:27:45Z

…-literal

Fixes #133618

…-literal Fixes llvm#133618

llvmbot · 2025-03-30T12:28:23Z

@llvm/pr-subscribers-clang-tools-extra

Author: Carlos Galvez (carlosgalvezp)

Changes

…-literal

Fixes #133618

Full diff: https://github.com/llvm/llvm-project/pull/133636.diff

3 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp (+6-5)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
(modified) clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp (+2-1)

diff --git a/clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp b/clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp
index 24674a407cb36..1be0d6cc23935 100644
--- a/clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp
@@ -58,7 +58,7 @@ bool containsEscapedCharacters(const MatchFinder::MatchResult &Result,
       *Result.SourceManager, Result.Context->getLangOpts());
   StringRef Text = Lexer::getSourceText(CharRange, *Result.SourceManager,
                                         Result.Context->getLangOpts());
-  if (Text.empty() || isRawStringLiteral(Text))
+  if (Text.empty() || !Text.contains('"') || isRawStringLiteral(Text))
     return false;
 
   return containsEscapes(Text, R"('\"?x01)");
@@ -156,14 +156,14 @@ static bool compareStringLength(StringRef Replacement,
                                 const SourceManager &SM,
                                 const LangOptions &LangOpts) {
   return Replacement.size() <=
-         Lexer::MeasureTokenLength(Literal->getBeginLoc(), SM, LangOpts);
+         Lexer::MeasureTokenLength(SM.getSpellingLoc(Literal->getBeginLoc()), SM, LangOpts);
 }
 
 void RawStringLiteralCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Literal = Result.Nodes.getNodeAs<StringLiteral>("lit");
-  if (Literal->getBeginLoc().isMacroID())
-    return;
   const SourceManager &SM = *Result.SourceManager;
+  if (SM.getSpellingLoc(Literal->getBeginLoc()).isMacroID())
+    return;
   const LangOptions &LangOpts = getLangOpts();
   if (containsEscapedCharacters(Result, Literal, DisallowedChars)) {
     const std::string Replacement =
@@ -172,7 +172,8 @@ void RawStringLiteralCheck::check(const MatchFinder::MatchResult &Result) {
         compareStringLength(Replacement, Literal, SM, LangOpts)) {
       diag(Literal->getBeginLoc(),
            "escaped string literal can be written as a raw string literal")
-          << FixItHint::CreateReplacement(Literal->getSourceRange(),
+          << FixItHint::CreateReplacement(SourceRange(SM.getSpellingLoc(Literal->getBeginLoc()),
+                                                      SM.getSpellingLoc(Literal->getEndLoc())),
                                           Replacement);
     }
   }
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6cb8d572d3a78..c7c5ac75986be 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -181,6 +181,10 @@ Changes in existing checks
   <clang-tidy/checks/modernize/use-std-numbers>` check to support math
   functions of different precisions.
 
+- Improved :doc:`modernize-raw-string-literal
+  <clang-tidy/checks/modernize/raw-string-literal>` check to detect string
+  literals passed into macros.
+
 - Improved :doc:`performance-move-const-arg
   <clang-tidy/checks/performance/move-const-arg>` check by fixing false
   negatives on ternary operators calling ``std::move``.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp
index 5856b8882574a..fc4e966e92e07 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp
@@ -105,7 +105,8 @@ char const *const StringizedMacroArgument = HAT(foo\\bar);
 
 #define SUBST(lit_) lit_
 char const *const MacroArgument = SUBST("foo\\bar");
-// FIXME: We should be able to replace this string literal macro argument
+// CHECK-MESSAGES: :[[@LINE-1]]:41: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const MacroArgument = SUBST(R"(foo\bar)");{{$}}
 
 template <typename T>
 void fn(char const *const Arg) {

llvmbot · 2025-03-30T12:28:23Z

@llvm/pr-subscribers-clang-tidy

Author: Carlos Galvez (carlosgalvezp)

Changes

…-literal

Fixes #133618

Full diff: https://github.com/llvm/llvm-project/pull/133636.diff

3 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp (+6-5)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
(modified) clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp (+2-1)

diff --git a/clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp b/clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp
index 24674a407cb36..1be0d6cc23935 100644
--- a/clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp
@@ -58,7 +58,7 @@ bool containsEscapedCharacters(const MatchFinder::MatchResult &Result,
       *Result.SourceManager, Result.Context->getLangOpts());
   StringRef Text = Lexer::getSourceText(CharRange, *Result.SourceManager,
                                         Result.Context->getLangOpts());
-  if (Text.empty() || isRawStringLiteral(Text))
+  if (Text.empty() || !Text.contains('"') || isRawStringLiteral(Text))
     return false;
 
   return containsEscapes(Text, R"('\"?x01)");
@@ -156,14 +156,14 @@ static bool compareStringLength(StringRef Replacement,
                                 const SourceManager &SM,
                                 const LangOptions &LangOpts) {
   return Replacement.size() <=
-         Lexer::MeasureTokenLength(Literal->getBeginLoc(), SM, LangOpts);
+         Lexer::MeasureTokenLength(SM.getSpellingLoc(Literal->getBeginLoc()), SM, LangOpts);
 }
 
 void RawStringLiteralCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Literal = Result.Nodes.getNodeAs<StringLiteral>("lit");
-  if (Literal->getBeginLoc().isMacroID())
-    return;
   const SourceManager &SM = *Result.SourceManager;
+  if (SM.getSpellingLoc(Literal->getBeginLoc()).isMacroID())
+    return;
   const LangOptions &LangOpts = getLangOpts();
   if (containsEscapedCharacters(Result, Literal, DisallowedChars)) {
     const std::string Replacement =
@@ -172,7 +172,8 @@ void RawStringLiteralCheck::check(const MatchFinder::MatchResult &Result) {
         compareStringLength(Replacement, Literal, SM, LangOpts)) {
       diag(Literal->getBeginLoc(),
            "escaped string literal can be written as a raw string literal")
-          << FixItHint::CreateReplacement(Literal->getSourceRange(),
+          << FixItHint::CreateReplacement(SourceRange(SM.getSpellingLoc(Literal->getBeginLoc()),
+                                                      SM.getSpellingLoc(Literal->getEndLoc())),
                                           Replacement);
     }
   }
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6cb8d572d3a78..c7c5ac75986be 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -181,6 +181,10 @@ Changes in existing checks
   <clang-tidy/checks/modernize/use-std-numbers>` check to support math
   functions of different precisions.
 
+- Improved :doc:`modernize-raw-string-literal
+  <clang-tidy/checks/modernize/raw-string-literal>` check to detect string
+  literals passed into macros.
+
 - Improved :doc:`performance-move-const-arg
   <clang-tidy/checks/performance/move-const-arg>` check by fixing false
   negatives on ternary operators calling ``std::move``.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp
index 5856b8882574a..fc4e966e92e07 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp
@@ -105,7 +105,8 @@ char const *const StringizedMacroArgument = HAT(foo\\bar);
 
 #define SUBST(lit_) lit_
 char const *const MacroArgument = SUBST("foo\\bar");
-// FIXME: We should be able to replace this string literal macro argument
+// CHECK-MESSAGES: :[[@LINE-1]]:41: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const MacroArgument = SUBST(R"(foo\bar)");{{$}}
 
 template <typename T>
 void fn(char const *const Arg) {

github-actions · 2025-03-30T12:31:21Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp -- clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp

View the diff from clang-format here.

diff --git a/clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp b/clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp
index 1be0d6cc2..b7fad9193 100644
--- a/clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp
@@ -156,7 +156,8 @@ static bool compareStringLength(StringRef Replacement,
                                 const SourceManager &SM,
                                 const LangOptions &LangOpts) {
   return Replacement.size() <=
-         Lexer::MeasureTokenLength(SM.getSpellingLoc(Literal->getBeginLoc()), SM, LangOpts);
+         Lexer::MeasureTokenLength(SM.getSpellingLoc(Literal->getBeginLoc()),
+                                   SM, LangOpts);
 }
 
 void RawStringLiteralCheck::check(const MatchFinder::MatchResult &Result) {
@@ -172,9 +173,10 @@ void RawStringLiteralCheck::check(const MatchFinder::MatchResult &Result) {
         compareStringLength(Replacement, Literal, SM, LangOpts)) {
       diag(Literal->getBeginLoc(),
            "escaped string literal can be written as a raw string literal")
-          << FixItHint::CreateReplacement(SourceRange(SM.getSpellingLoc(Literal->getBeginLoc()),
-                                                      SM.getSpellingLoc(Literal->getEndLoc())),
-                                          Replacement);
+          << FixItHint::CreateReplacement(
+                 SourceRange(SM.getSpellingLoc(Literal->getBeginLoc()),
+                             SM.getSpellingLoc(Literal->getEndLoc())),
+                 Replacement);
     }
   }
 }

EugeneZelenko · 2025-03-30T14:08:24Z

clang-tools-extra/docs/ReleaseNotes.rst

  <clang-tidy/checks/modernize/use-std-numbers>` check to support math
  functions of different precisions.

+- Improved :doc:`modernize-raw-string-literal


Please keep alphabetical order (by check name) in this list.

HerrCai0907 · 2025-03-30T15:21:30Z

clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp

 #define HAT(rabbit_) #rabbit_ "foo\\bar"
 char const *const StringizedMacroArgument = HAT(foo\\bar);

 #define SUBST(lit_) lit_


what about concat string in macro?
e.g.

#define SUBST(a,b) a#b char const *const MacroArgument = SUBST("foo\\bar","cd");

Good point, this doesn't work. It wants to create a replacement "R\"(foo\\bar\\baz\\bazz\\foo\"cd\")\"" which is non sense.

Most likely macros should be handled in a PPCallback instead, thus it's better to drop this PR and implement this feature there instead.

carlosgalvezp · 2025-03-30T18:40:39Z

This approach does not scale for macros, it should rather be implemented in a PPCallback instead.

LegalizeAdulthood · 2025-03-31T04:48:28Z

clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp

  StringRef Text = Lexer::getSourceText(CharRange, *Result.SourceManager,
                                        Result.Context->getLangOpts());
-  if (Text.empty() || isRawStringLiteral(Text))
+  if (Text.empty() || !Text.contains('"') || isRawStringLiteral(Text))


Why is this necessary?

It picked up on this:

#define TRICK(arg_) #arg_ char const *const MacroBody = TRICK(foo\\bar);

Since foo\\bar does not have quotes, the assertion inside isRawStringLiteral failed.

So... shouldn't the check include something that says this is only when the text is a macro argument to #, or am I missing something?

I'm not sure I understand the question. Do you mean that we should update the user-facing documentation of the check? Or that we should modify the implementation?

I mean this is a blanket-wide check to see if the text contains a double quote character ("), but isn't contextualized to the specific case that matters -- when the text is supplied as a macro argument to the preprocessor string-ize operator #. I don't see how adding this condition applies only to that situation.

Yeah, that won't work. Do note this PR is abandoned :)

Yes, I just wanted to understand the change as written to make sure I wasn't missing something :). FYI, I am the one who contributed this check and a weakness of several early checks I wrote is that they don't play well with macros and this was one of them.

[clang-tidy] Detect string literals in macros in modernize-raw-string…

2baffdb

…-literal Fixes llvm#133618

llvmbot added clang-tools-extra clang-tidy labels Mar 30, 2025

carlosgalvezp requested review from HerrCai0907 and PiotrZSL March 30, 2025 12:28

EugeneZelenko reviewed Mar 30, 2025

View reviewed changes

HerrCai0907 reviewed Mar 30, 2025

View reviewed changes

carlosgalvezp closed this Mar 30, 2025

LegalizeAdulthood reviewed Mar 31, 2025

View reviewed changes

[clang-tidy] Detect string literals in macros in modernize-raw-string… #133636

[clang-tidy] Detect string literals in macros in modernize-raw-string… #133636

Uh oh!

Conversation

carlosgalvezp commented Mar 30, 2025

Uh oh!

llvmbot commented Mar 30, 2025

Uh oh!

llvmbot commented Mar 30, 2025

Uh oh!

github-actions bot commented Mar 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosgalvezp commented Mar 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LegalizeAdulthood Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LegalizeAdulthood Apr 2, 2025 •

edited

Loading