Re: [PATCH] D19331: [Clang-tidy] Fix for crash in modernize-raw-string-literal check

[Alexander Kornienko via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL266992: [Clang-tidy] Fix for crash in 
modernize-raw-string-literal check (authored by alexfh).

Changed prior to commit:
  http://reviews.llvm.org/D19331?vs=54401=54504#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D19331

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/RawStringLiteralCheck.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-raw-string-literal.cpp

Index: clang-tools-extra/trunk/clang-tidy/modernize/RawStringLiteralCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/RawStringLiteralCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/RawStringLiteralCheck.cpp
@@ -108,7 +108,8 @@
 }
 
 void RawStringLiteralCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(stringLiteral().bind("lit"), this);
+  Finder->addMatcher(
+  stringLiteral(unless(hasParent(predefinedExpr(.bind("lit"), this);
 }
 
 void RawStringLiteralCheck::check(const MatchFinder::MatchResult ) {
Index: clang-tools-extra/trunk/test/clang-tidy/modernize-raw-string-literal.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-raw-string-literal.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-raw-string-literal.cpp
@@ -91,6 +91,10 @@
 // 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);
 


Index: clang-tools-extra/trunk/clang-tidy/modernize/RawStringLiteralCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/RawStringLiteralCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/RawStringLiteralCheck.cpp
@@ -108,7 +108,8 @@
 }
 
 void RawStringLiteralCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(stringLiteral().bind("lit"), this);
+  Finder->addMatcher(
+  stringLiteral(unless(hasParent(predefinedExpr(.bind("lit"), this);
 }
 
 void RawStringLiteralCheck::check(const MatchFinder::MatchResult ) {
Index: clang-tools-extra/trunk/test/clang-tidy/modernize-raw-string-literal.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-raw-string-literal.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-raw-string-literal.cpp
@@ -91,6 +91,10 @@
 // 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);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19331: [Clang-tidy] Fix for crash in modernize-raw-string-literal check

[Marek via cfe-commits]

edyp87 added a comment.

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 ) {
 [...]
 if (Literal->getLocStart().isMacroID())
   return;
 [...]
  }

Thank you for quick review!


http://reviews.llvm.org/D19331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19331: [Clang-tidy] Fix for crash in modernize-raw-string-literal check

[Marek via cfe-commits]

edyp87 marked an inline comment as done.
edyp87 added a comment.

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  col:19 function 'const char *const' 
> callinit

>`-ImplicitCastExpr 0x2b274c0  'const char *' 

>  `-PredefinedExpr 0x2b27470  'const char [1]' lvalue __FUNCTION__

>`-StringLiteral 0x2b27448  'const char [1]' lvalue ""

> 

> Valid case:

> 

>   -VarDecl 0x2b26660  col:19 HexPrintable 'const char 
> *const' callinit

>`-ImplicitCastExpr 0x2b26718  'const char *' 

>  `-StringLiteral 0x2b266b8  'const char [3]' lvalue "@\\"


For `StringExpr` 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.


http://reviews.llvm.org/D19331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19331: [Clang-tidy] Fix for crash in modernize-raw-string-literal check

[Marek via cfe-commits]

edyp87 removed rL LLVM as the repository for this revision.
edyp87 updated this revision to Diff 54401.
edyp87 added a comment.

Extended diff range + removed unnecessary variable.


http://reviews.llvm.org/D19331

Files:
  clang-tidy/modernize/RawStringLiteralCheck.cpp
  test/clang-tidy/modernize-raw-string-literal.cpp

Index: test/clang-tidy/modernize-raw-string-literal.cpp
===
--- test/clang-tidy/modernize-raw-string-literal.cpp
+++ test/clang-tidy/modernize-raw-string-literal.cpp
@@ -91,6 +91,10 @@
 // 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);
 
Index: clang-tidy/modernize/RawStringLiteralCheck.cpp
===
--- clang-tidy/modernize/RawStringLiteralCheck.cpp
+++ clang-tidy/modernize/RawStringLiteralCheck.cpp
@@ -108,7 +108,10 @@
 }
 
 void RawStringLiteralCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(stringLiteral().bind("lit"), this);
+
+  Finder->addMatcher(
+  stringLiteral(unless(hasParent(predefinedExpr(.bind("lit"),
+  this);
 }
 
 void RawStringLiteralCheck::check(const MatchFinder::MatchResult ) {


Index: test/clang-tidy/modernize-raw-string-literal.cpp
===
--- test/clang-tidy/modernize-raw-string-literal.cpp
+++ test/clang-tidy/modernize-raw-string-literal.cpp
@@ -91,6 +91,10 @@
 // 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);
 
Index: clang-tidy/modernize/RawStringLiteralCheck.cpp
===
--- clang-tidy/modernize/RawStringLiteralCheck.cpp
+++ clang-tidy/modernize/RawStringLiteralCheck.cpp
@@ -108,7 +108,10 @@
 }
 
 void RawStringLiteralCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(stringLiteral().bind("lit"), this);
+
+  Finder->addMatcher(
+  stringLiteral(unless(hasParent(predefinedExpr(.bind("lit"),
+  this);
 }
 
 void RawStringLiteralCheck::check(const MatchFinder::MatchResult ) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19331: [Clang-tidy] Fix for crash in modernize-raw-string-literal check

[Alexander Kornienko via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

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.


http://reviews.llvm.org/D19331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19331: [Clang-tidy] Fix for crash in modernize-raw-string-literal check

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

1. Please generate diffs with full context when sending patches. Use any of the 
methods described in http://llvm.org/docs/Phabricator.html.



Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:111
@@ -110,2 +110,3 @@
 void RawStringLiteralCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(stringLiteral().bind("lit"), this);
+  const auto IsNotPredefinedExpr = unless(hasParent(predefinedExpr()));
+

No need in this variable.


Repository:
  rL LLVM

http://reviews.llvm.org/D19331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19331: [Clang-tidy] Fix for crash in modernize-raw-string-literal check

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

(hit Submit early...)

2. 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!


Repository:
  rL LLVM

http://reviews.llvm.org/D19331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits