Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
carlosgalvezp wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

@LegalizeAdulthood LegalizeAdulthood Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

Copy link
Contributor Author

@carlosgalvezp carlosgalvezp Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@LegalizeAdulthood LegalizeAdulthood Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@carlosgalvezp carlosgalvezp Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@LegalizeAdulthood LegalizeAdulthood Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@carlosgalvezp carlosgalvezp Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@LegalizeAdulthood LegalizeAdulthood Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return false;

return containsEscapes(Text, R"('\"?x01)");
Expand Down Expand Up @@ -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 =
Expand All @@ -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);
}
}
Expand Down
4 changes: 4 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@EugeneZelenko EugeneZelenko Mar 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<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``.
Expand Down
3 changes: 2 additions & 1 deletion clang-tools-extra/test/clang-tidy/checkers/modernize/raw-string-literal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ char const *const StringizedMacroArgument = HAT(foo\\bar);

#define SUBST(lit_) lit_
Copy link
Contributor

@HerrCai0907 HerrCai0907 Mar 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about concat string in macro?
e.g.

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

Copy link
Contributor Author

@carlosgalvezp carlosgalvezp Mar 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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) {
Expand Down
Loading