This is an archive of the discontinued LLVM Phabricator instance.

[Clang-tidy] Fix for crash in modernize-raw-string-literal check
ClosedPublic

Authored by edyp87 on Apr 20 2016, 9:48 AM.

Details

Summary

Clang-tidy modernize-raw-string-literal check crashes on run-time assert while it is evaluating compiler predefined identifiers such as

  • FUNCTION
  • func
  • PRETTY_FUNCTION

Check is asserting because it cannot find opening quote for such string literal. It occurs only on debug build config.
I think that it would be good to prune such cases by crossing off predefined expressions - there is no need to evaluate such matches.

Diff Detail

Event Timeline

edyp87 updated this revision to Diff 54385.Apr 20 2016, 9:48 AM
edyp87 retitled this revision from to Fix for crash in modernize-raw-string-literal check.
edyp87 updated this object.
edyp87 added a reviewer: LegalizeAdulthood.
Eugene.Zelenko retitled this revision from Fix for crash in modernize-raw-string-literal check to [Clang-tidy] Fix for crash in modernize-raw-string-literal check.Apr 20 2016, 10:50 AM
Eugene.Zelenko added a reviewer: alexfh.
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a subscriber: cfe-commits.
alexfh edited edge metadata.Apr 20 2016, 11:46 AM
  1. Please generate diffs with full context when sending patches. Use any of the methods described in http://llvm.org/docs/Phabricator.html.
clang-tidy/modernize/RawStringLiteralCheck.cpp
111

No need in this variable.

(hit Submit early...)

  1. How does AST look for these test cases? I wonder whether there are any similar cases not covered by PredefinedExpr.

And thank you for the patch!

alexfh requested changes to this revision.Apr 20 2016, 11:48 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 20 2016, 11:48 AM
edyp87 updated this revision to Diff 54401.Apr 20 2016, 1:09 PM
edyp87 edited edge metadata.
edyp87 removed rL LLVM as the repository for this revision.

Extended diff range + removed unnecessary variable.

edyp87 marked an inline comment as done.EditedApr 20 2016, 1:27 PM
  1. Extended diff has been generated - sorry, I am new to Phabricator.
  2. AST for this case looks like this:

AST for crashing case:
-VarDecl 0x2b27370 <line:96:1, col:28> col:19 function 'const char *const' callinit

`-ImplicitCastExpr 0x2b274c0 <col:28> 'const char *' <ArrayToPointerDecay>
  `-PredefinedExpr 0x2b27470 <col:28> 'const char [1]' lvalue __FUNCTION__
    `-StringLiteral 0x2b27448 <col:28> 'const char [1]' lvalue ""

Valid case:
-VarDecl 0x2b26660 <line:90:1, col:32> col:19 HexPrintable 'const char *const' callinit

`-ImplicitCastExpr 0x2b26718 <col:32> 'const char *' <ArrayToPointerDecay>
  `-StringLiteral 0x2b266b8 <col:32> 'const char [3]' lvalue "@\\"

For StringLiteral whose parent is PredefinedExpr Lexer::getSourceText returns this expr literally (__FUNCTION__) instead of evaluated function name.
I was wondering whether there is another case which results in such assert (lack of quote in string) but I could not came with an idea of such scenario.
Another approach would be just returning from check() method while evaluating "quote-less" string but I thought that it would be less elegant.

alexfh accepted this revision.Apr 20 2016, 3:01 PM
alexfh edited edge metadata.

Thank you for the explanation! The change looks good now. Do you need me to submit the patch for you?

As for other cases that can lead to this, it might be possible to achieve the same effect using macros. However, we could address this in a separate patch.

This revision is now accepted and ready to land.Apr 20 2016, 3:01 PM

Yes, please apply this patch for me.

As for macro cases - I also thought about this but check's author has taken care of macros in check() method :

void RawStringLiteralCheck::check(const MatchFinder::MatchResult &Result) {
   [...]
   if (Literal->getLocStart().isMacroID())
     return;
   [...]
}

Thank you for quick review!

This revision was automatically updated to reflect the committed changes.