[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Mitchell via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1315f4e009b0: [clang-tidy] Fix 
readability-redundant-string-init for c++17/c++2a (authored by 
mitchell-stellar).

Changed prior to commit:
  https://reviews.llvm.org/D69238?vs=229651=230039#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238

Files:
  clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
@@ -1,4 +1,8 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s 
readability-redundant-string-init %t
+// RUN: %check_clang_tidy -std=c++11,c++14 %s 
readability-redundant-string-init %t \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: readability-redundant-string-init.StringNames, \
+// RUN:   value: '::std::basic_string;our::TestString'}] \
+// RUN: }"
 // FIXME: Fix the checker to work in C++17 mode.
 
 namespace std {
@@ -131,6 +135,11 @@
   // CHECK-FIXES: std::string a, b, c;
 
   std::string d = "u", e = "u", f = "u";
+
+  std::string g = "u", h = "", i = "uuu", j = "", k;
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: redundant string initialization
+  // CHECK-MESSAGES: [[@LINE-2]]:43: warning: redundant string initialization
+  // CHECK-FIXES: std::string g = "u", h, i = "uuu", j, k;
 }
 
 // These cases should not generate warnings.
Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s 
readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
Index: clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
@@ -73,7 +73,7 @@
   namedDecl(
   varDecl(
   hasType(hasUnqualifiedDesugaredType(recordType(
-  hasDeclaration(cxxRecordDecl(hasName("basic_string")),
+  hasDeclaration(cxxRecordDecl(hasStringTypeName),
   hasInitializer(expr(ignoringImplicit(anyOf(
   EmptyStringCtorExpr, EmptyStringCtorExprWithTemporaries)
   .bind("vardecl"),
@@ -82,11 +82,12 @@
 }
 
 void RedundantStringInitCheck::check(const MatchFinder::MatchResult ) {
-  const auto *CtorExpr = Result.Nodes.getNodeAs("expr");
-  const auto *Decl = Result.Nodes.getNodeAs("decl");
-  diag(CtorExpr->getExprLoc(), "redundant string initialization")
-  << FixItHint::CreateReplacement(CtorExpr->getSourceRange(),
-  Decl->getName());
+  const auto *VDecl = Result.Nodes.getNodeAs("vardecl");
+  // VarDecl's getSourceRange() spans 'string foo = ""' or 'string bar("")'.
+  // So start at getLocation() to span just 'foo = ""' or 'bar("")'.
+  SourceRange ReplaceRange(VDecl->getLocation(), VDecl->getEndLoc());
+  diag(VDecl->getLocation(), "redundant string initialization")
+  << FixItHint::CreateReplacement(ReplaceRange, VDecl->getName());
 }
 
 } // namespace readability


Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
@@ -1,4 +1,8 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t
+// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: readability-redundant-string-init.StringNames, \
+// RUN:   value: '::std::basic_string;our::TestString'}] \
+// RUN: }"
 // FIXME: Fix the checker to work in C++17 mode.
 
 namespace std {
@@ -131,6 +135,11 @@
   // CHECK-FIXES: std::string a, b, c;
 
   std::string d = "u", e = "u", f = "u";
+
+  std::string g 

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Mitchell via Phabricator via cfe-commits]

mitchell-stellar reopened this revision.
mitchell-stellar added a comment.
This revision is now accepted and ready to land.

Reopening in order to correct the accidentally swapped commits between this 
ticket and https://reviews.llvm.org/D70165.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Mitchell via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG06f3dabe4a2e: [clang-tidy] Fix 
readability-redundant-string-init for c++17/c++2a (authored by 
mitchell-stellar).

Changed prior to commit:
  https://reviews.llvm.org/D69238?vs=228556=229651#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238

Files:
  clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst


Index: clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
@@ -27,6 +27,14 @@
 
If set to non-zero, this check will not diagnose destructors. Default is 
`0`.
 
+.. option:: AllowOverrideAndFinal
+
+   If set to non-zero, this check will not diagnose ``override`` as redundant
+   with ``final``. This is useful when code will be compiled by a compiler with
+   warning/error checking flags requiring ``override`` explicitly on overriden
+   members, such as ``gcc -Wsuggest-override``/``gcc 
-Werror=suggest-override``.
+   Default is `0`.
+
 .. option:: OverrideSpelling
 
Specifies a macro to use instead of ``override``. This is useful when
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -164,6 +164,12 @@
   Finds non-static member functions that can be made ``const``
   because the functions don't use ``this`` in a non-const way.
 
+- Improved :doc:`modernize-use-override
+  ` check.
+
+  The check now supports the ``AllowOverrideAndFinal`` option to eliminate
+  conflicts with ``gcc -Wsuggest-override`` or ``gcc 
-Werror=suggest-override``.
+
 Improvements to include-fixer
 -
 
Index: clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
===
--- clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
+++ clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
@@ -26,6 +26,7 @@
 
 private:
   const bool IgnoreDestructors;
+  const bool AllowOverrideAndFinal;
   const std::string OverrideSpelling;
   const std::string FinalSpelling;
 };
Index: clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
@@ -20,11 +20,13 @@
 UseOverrideCheck::UseOverrideCheck(StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   IgnoreDestructors(Options.get("IgnoreDestructors", false)),
+  AllowOverrideAndFinal(Options.get("AllowOverrideAndFinal", false)),
   OverrideSpelling(Options.get("OverrideSpelling", "override")),
   FinalSpelling(Options.get("FinalSpelling", "final")) {}
 
 void UseOverrideCheck::storeOptions(ClangTidyOptions::OptionMap ) {
   Options.store(Opts, "IgnoreDestructors", IgnoreDestructors);
+  Options.store(Opts, "AllowOverrideAndFinal", AllowOverrideAndFinal);
   Options.store(Opts, "OverrideSpelling", OverrideSpelling);
   Options.store(Opts, "FinalSpelling", FinalSpelling);
 }
@@ -103,7 +105,8 @@
   bool OnlyVirtualSpecified = HasVirtual && !HasOverride && !HasFinal;
   unsigned KeywordCount = HasVirtual + HasOverride + HasFinal;
 
-  if (!OnlyVirtualSpecified && KeywordCount == 1)
+  if ((!OnlyVirtualSpecified && KeywordCount == 1) ||
+  (!HasVirtual && HasOverride && HasFinal && AllowOverrideAndFinal))
 return; // Nothing to do.
 
   std::string Message;
@@ -113,8 +116,9 @@
 Message = "annotate this function with '%0' or (rarely) '%1'";
   } else {
 StringRef Redundant =
-HasVirtual ? (HasOverride && HasFinal ? "'virtual' and '%0' are"
-  : "'virtual' is")
+HasVirtual ? (HasOverride && HasFinal && !AllowOverrideAndFinal
+  ? "'virtual' and '%0' are"
+  : "'virtual' is")
: "'%0' is";
 StringRef Correct = HasFinal ? "'%1'" : "'%0'";
 
@@ -211,7 +215,7 @@
 Diag << FixItHint::CreateInsertion(InsertLoc, ReplacementText);
   }
 
-  if (HasFinal && HasOverride) {
+  if (HasFinal && HasOverride && !AllowOverrideAndFinal) {
 SourceLocation OverrideLoc = 
Method->getAttr()->getLocation();
 Diag << FixItHint::CreateRemoval(
 CharSourceRange::getTokenRange(OverrideLoc, OverrideLoc));


Index: clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

In D69238#1740914 , @gribozavr2 wrote:

> Thanks! Do you have commit access or do you need me to commit for you?


I don't have commit access, if you could commit it for me that would be great. 
Thanks!


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Dmitri Gribenko via Phabricator via cfe-commits]

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

Thanks! Do you have commit access or do you need me to commit for you?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

In D69238#1739627 , @NoQ wrote:

> Clang's `-ast-dump` 
> .


Wow. That makes this so much easier... Thank you so much!

Looking at the AST showed that the `CXXConstructExpr`s that 
`readability-redundant-string-init` previously relied upon for `SourceRange` 
were being elided in C++17/2a, but that `VarDecl`s consistently held all the 
right info across all C++ modes. So I completely rewrote this patch to get the 
`SourceRange` from the `VarDecl`. All tests pass.

In case anyone cares or for posterity, what follows are a bunch of probably 
unnecessary details.

I made a super minimal readability-redundant-string-init.cpp:

  namespace std {
  struct string {
string();
string(const char *);
  };
  }
  
  void f() {
std::string a = "", b = "foo", c, d(""), e("bar");
  }

Running `clang -Xclang -ast-dump -std=c++11 
readability-redundant-string-init.cpp` yields:

  `-FunctionDecl 0x27a33bfd610  line:8:6 f 'void ()'
`-CompoundStmt 0x27a33bf6c78 
  `-DeclStmt 0x27a33bf6c60 
|-VarDecl 0x27a33bfd788  col:15 a 
'std::string':'std::string' cinit
| `-ExprWithCleanups 0x27a33bfddd0  
'std::string':'std::string'
|   `-CXXConstructExpr 0x27a33bfdda0  
'std::string':'std::string' 'void (std::string &&) noexcept' elidable
| `-MaterializeTemporaryExpr 0x27a33bfdd48  
'std::string':'std::string' xvalue
|   `-ImplicitCastExpr 0x27a33bfdc20  
'std::string':'std::string' 
| `-CXXConstructExpr 0x27a33bfdbf0  
'std::string':'std::string' 'void (const char *)'
|   `-ImplicitCastExpr 0x27a33bfdbd8  'const char *' 

| `-StringLiteral 0x27a33bfd868  'const char [1]' 
lvalue ""
|-VarDecl 0x27a33bfde08  col:23 b 
'std::string':'std::string' cinit
| `-ExprWithCleanups 0x27a33bfdfb0  
'std::string':'std::string'
|   `-CXXConstructExpr 0x27a33bfdf80  
'std::string':'std::string' 'void (std::string &&) noexcept' elidable
| `-MaterializeTemporaryExpr 0x27a33bfdf68  
'std::string':'std::string' xvalue
|   `-ImplicitCastExpr 0x27a33bfdf50  
'std::string':'std::string' 
| `-CXXConstructExpr 0x27a33bfdf20  
'std::string':'std::string' 'void (const char *)'
|   `-ImplicitCastExpr 0x27a33bfdf08  'const char *' 

| `-StringLiteral 0x27a33bfdee8  'const char [4]' 
lvalue "foo"
|-VarDecl 0x27a33bfdfe8  col:34 c 
'std::string':'std::string' callinit
| `-CXXConstructExpr 0x27a33bfe050  'std::string':'std::string' 
'void ()'
|-VarDecl 0x27a33bfe098  col:37 d 
'std::string':'std::string' callinit
| `-CXXConstructExpr 0x27a33bf6af0  
'std::string':'std::string' 'void (const char *)'
|   `-ImplicitCastExpr 0x27a33bf6ad8  'const char *' 

| `-StringLiteral 0x27a33bf6aa0  'const char [1]' lvalue ""
`-VarDecl 0x27a33bf6b40  col:44 e 
'std::string':'std::string' callinit
  `-CXXConstructExpr 0x27a33bf6c00  
'std::string':'std::string' 'void (const char *)'
`-ImplicitCastExpr 0x27a33bf6be8  'const char *' 

  `-StringLiteral 0x27a33bf6ba8  'const char [4]' lvalue 
"bar"

With -std=c++11, all of the outer `CXXConstructExpr` have the correct range to 
replace with just the variable names, which explains why all is fine with C++11.

Then running `clang -Xclang -ast-dump -std=c++17 
readability-redundant-string-init.cpp` yields:

  `-FunctionDecl 0x2b8e6ac96b0  line:8:6 f 'void ()'
`-CompoundStmt 0x2b8e6acc608 
  `-DeclStmt 0x2b8e6acc5f0 
|-VarDecl 0x2b8e6ac9828  col:15 a 
'std::string':'std::string' cinit
| `-ImplicitCastExpr 0x2b8e6ac9cc0  'std::string':'std::string' 

|   `-CXXConstructExpr 0x2b8e6ac9c90  
'std::string':'std::string' 'void (const char *)'
| `-ImplicitCastExpr 0x2b8e6ac9c78  'const char *' 

|   `-StringLiteral 0x2b8e6ac9908  'const char [1]' lvalue 
""
|-VarDecl 0x2b8e6ac9e08  col:23 b 
'std::string':'std::string' cinit
| `-ImplicitCastExpr 0x2b8e6ac9f50  'std::string':'std::string' 

|   `-CXXConstructExpr 0x2b8e6ac9f20  
'std::string':'std::string' 'void (const char *)'
| `-ImplicitCastExpr 0x2b8e6ac9f08  'const char *' 

|   `-StringLiteral 0x2b8e6ac9ee8  'const char [4]' lvalue 
"foo"
|-VarDecl 0x2b8e6ac9f88  col:34 c 
'std::string':'std::string' callinit
| `-CXXConstructExpr 0x2b8e6ac9ff0  'std::string':'std::string' 
'void ()'
|-VarDecl 0x2b8e6aca038  col:37 d 
'std::string':'std::string' callinit
| `-CXXConstructExpr 0x2b8e6aca0f0  
'std::string':'std::string' 'void (const char *)'
|   `-ImplicitCastExpr 0x2b8e6aca0d8  'const char *' 

| `-StringLiteral 0x2b8e6aca0a0  'const char [1]' lvalue ""
`-VarDecl 0x2b8e6acc4d0  col:44 e 

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 228556.
poelmanc edited the summary of this revision.

Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238

Files:
  clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s 
readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
@@ -131,6 +130,11 @@
   // CHECK-FIXES: std::string a, b, c;
 
   std::string d = "u", e = "u", f = "u";
+
+  std::string g = "u", h = "", i = "uuu", j = "", k;
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: redundant string initialization
+  // CHECK-MESSAGES: [[@LINE-2]]:43: warning: redundant string initialization
+  // CHECK-FIXES: std::string g = "u", h, i = "uuu", j, k;
 }
 
 // These cases should not generate warnings.
Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s 
readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
Index: clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
@@ -46,23 +46,23 @@
   // string bar("");
   Finder->addMatcher(
   namedDecl(
-  varDecl(hasType(hasUnqualifiedDesugaredType(recordType(
-  
hasDeclaration(cxxRecordDecl(hasName("basic_string")),
-  hasInitializer(expr(ignoringImplicit(anyOf(
-  EmptyStringCtorExpr,
-  EmptyStringCtorExprWithTemporaries)))
- .bind("expr"))),
-  unless(parmVarDecl()))
-  .bind("decl"),
+  varDecl(
+  hasType(hasUnqualifiedDesugaredType(recordType(
+  hasDeclaration(cxxRecordDecl(hasName("basic_string")),
+  hasInitializer(expr(ignoringImplicit(anyOf(
+  EmptyStringCtorExpr, EmptyStringCtorExprWithTemporaries)
+  .bind("vardecl"),
+  unless(parmVarDecl())),
   this);
 }
 
 void RedundantStringInitCheck::check(const MatchFinder::MatchResult ) {
-  const auto *CtorExpr = Result.Nodes.getNodeAs("expr");
-  const auto *Decl = Result.Nodes.getNodeAs("decl");
-  diag(CtorExpr->getExprLoc(), "redundant string initialization")
-  << FixItHint::CreateReplacement(CtorExpr->getSourceRange(),
-  Decl->getName());
+  const auto *VDecl = Result.Nodes.getNodeAs("vardecl");
+  // VarDecl's getSourceRange() spans 'string foo = ""' or 'string bar("")'.
+  // So start at getLocation() to span just 'foo = ""' or 'bar("")'.
+  SourceRange ReplaceRange(VDecl->getLocation(), VDecl->getEndLoc());
+  diag(VDecl->getLocation(), "redundant string initialization")
+  << FixItHint::CreateReplacement(ReplaceRange, VDecl->getName());
 }
 
 } // namespace readability


Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
@@ -131,6 +130,11 @@
   // CHECK-FIXES: std::string a, b, c;
 
   std::string d = "u", e = "u", f = "u";
+
+  std::string g = "u", h = "", i = "uuu", j = "", k;
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: redundant string initialization
+  // CHECK-MESSAGES: 

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D69238#1739618 , @poelmanc wrote:

> Is there a tool to print the AST? That would show whether the AST already has 
> some expression with the right SourceRange.


Clang's `-ast-dump` 
.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

In D69238#1739429 , @gribozavr2 wrote:

> If it is indeed the extra AST node for the elidable constructor, see if you 
> can use the `ignoringElidableConstructorCall` AST matcher to ignore it, 
> therefore smoothing over AST differences between language modes.


Thanks for the tip. Adding `ignoringElidableConstructorCall` in front of 
`cxxConstructExpr` for the `EmptyStringCtorExpr` and 
`EmptyStringCtorExprWithTemporaries` in RedundantStringInitCheck.cpp resulted 
in the checker no longer matching any of `std::string a = ""` lines, i.e. 
basically disabling the check for those types of lines.

Is there a tool to print the AST? That would show whether the AST already has 
some expression with the right SourceRange.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

In D69238#1736365 , @NoQ wrote:

> I suspect that it's not just the source range, but the whole AST for the 
> initializer is different, due to C++17 mandatory copy elision in the 
> equals-initializer syntax. Like, before C++17 it was a temporary constructor, 
> a temporary materialization (ironic!), and a copy constructor, but in C++17 
> and after it's a single direct constructor which looks exactly like the old 
> temporary constructor (except not being a `CXXTemporaryObjectExpr`). You're 
> most likely talking about //different construct-expressions// in different 
> language modes.
>
> That said, it should probably be possible to change the source range anyway 
> somehow.


Thanks to all the encouragement here, I spent a few more hours stepping through 
code and have found a one-line change to clang\lib\Sema\SemaInit.cpp:8053 that 
fixes this bug! Change:

  SourceLocation Loc = CurInit.get()->getBeginLoc();

to

  SourceLocation Loc = Kind.getLocation();

For `SK_UserConversion`, `CurInit` is set at SemaInit.cpp:7899 to `Args[0]`, 
i.e. the first argument to the constructor, which is `""` in this case. By 
changing `Loc` to `Kind.getLocation()`, the `BuildCXXConstructExpr` at 
SemaInit.cpp:8064 gets created with a SourceRange spanning `a = ""`. With just 
that change, the SourceRange for an expression like `std::string a = ""` 
becomes consistent across C++11/14/17/2a and 
`readability-redundant-string-init` tests pass in all C++ modes (so we can 
throw away my 70 lines of manual parsing code.)

I have no experience with the clang parsing code so I don't know what other 
effects this change might have or who else we might want to check with before 
committing this. Should I change my "diff" here to just that?

> Also i don't see any tests for multiple declarations in a single `DeclStmt`, 
> i.e.
> 
>   string A = "a", B = "";
> 
> 
> ... - which source range do you expect to have if you warn on `B`?

For that example I'd expect the warning source range to cover `B = ""`. 
`readability-redundant-string-init.cpp` does include tests almost like that: 
`std::string a = "", b = "", c = "";` warns on 3 separate source ranges, while 
`std::string d = "u", e = "u", f = "u";` doesn't warn at all. I'm happy to add 
a line that has some of each. I just added `std::string g = "u", h = "", i = 
"uuu", j = "", k;` and confirmed that it warns and fixes correctly (in C++11/14 
mode it should pass already; in C++17/2a mode it works correctly with the 
current patch.)


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

If it is indeed the extra AST node for the elidable constructor, see if you can 
use the `ignoringElidableConstructorCall` AST matcher to ignore it, therefore 
smoothing over AST differences between language modes.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

Just documenting here that I sent the following email to cfe-...@lists.llvm.org:

> When parsing a named declaration with an equals sign with clang -std 
> c++11/14, clang builds an initializer expression whose SourceRange covers 
> from the variable name through the end of the initial value:
> 
>   std::string foo = "bar";
>   ^~~
> 
> When parsing the same code with clang -std c++17/2x, the initializer 
> expression's SourceRange only includes the initial value itself, and not the 
> variable name or the equals sign:
> 
>   std::string foo = "bar";
> ^
> 
> If the string is initialized using parentheses rather than an equals sign, in 
> all of c++11/14/17/2x the initializer expression's SourceRange includes the 
> variable name and both parentheses:
> 
>   std::string foo("bar");
>   ^~
> 
> This difference has broken clang-tidy's readability-remove-redundant-string 
> for c++17 and c++2x, as described in at reviews.llvm.org/D69238 
> . Is this SourceRange difference 
> intentional, and if not does anyone have thoughts on how to make the 
> SourceRange consistent across C++ standards? Thanks!
> 
> P.S. I've tried stepping through clang-tidy parsing the `std::string a = "";` 
> line in 
> `clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp`
>  with -std=c++11 and again with -std=c++17. In both cases 
> `Sema::AddInitializerToDecl` seems to create a correct `InitializationKind` 
> with the `Locations[0]` properly pointing to the start of the variable name. 
> I'm not sure how or where that gets translated to the expression's 
> SourceRange later on in some way that presumably differs between c++11 and 
> c++17. If this SourceRange difference is unintentional and needs to be 
> tracked down and fixed, any tips on where to look would be appreciated.

I didn't receive any email responses, I believe it prompted @NoQ to look this 
over and comment - thanks!


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I suspect that it's not just the source range, but the whole AST for the 
initializer is different, due to C++17 mandatory copy elision in the 
equals-initializer syntax. Like, before C++17 it was a temporary constructor, a 
temporary materialization (ironic!), and a copy constructor, but in C++17 and 
after it's a single direct constructor which looks exactly like the old 
temporary constructor (except not being a `CXXTemporaryObjectExpr`). You're 
most likely talking about //different construct-expressions// in different 
language modes.

That said, it should probably be possible to change the source range anyway 
somehow.

Also i don't see any tests for multiple declarations in a single `DeclStmt`, 
i.e.

  string A = "a", B = "b";

... - which source range do you expect to have if you warn on `B`?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:67-70
+// With -std c++14 or earlier (!LangOps.CPlusPlus17), it was sufficient to
+// return CtorExpr->getSourceRange(). However, starting with c++17, parsing
+// the expression 'std::string Name = ""' results in a CtorExpr whose
+// SourceRange includes just '""' rather than the previous 'Name = ""'.

gribozavr2 wrote:
> poelmanc wrote:
> > aaron.ballman wrote:
> > > Are you sure that this behavioral difference isn't just a bug in Clang? I 
> > > can't think of why the source range should differ based on language mode, 
> > > so I wonder if the correct thing to do is fix how we calculate the source 
> > > range in Clang?
> > Thanks and no, I'm not sure at all. At the end of my summary paragraph I 
> > wrote:
> > 
> > > Or, is it possible that this change in SourceRange is an unintentional 
> > > difference in the parsing code? If so fixing that might make more sense.
> > 
> > Before starting this patch, I built a Debug build of LLVM and stepped 
> > through LLVM parsing code while running clang-tidy on the 
> > readability-redundant-string-init.cpp file. I set breakpoints and inspected 
> > variable values under both -std c++14 and -std c++17. The call stacks were 
> > clearly different. I could sometimes inspect character positions in the 
> > file and see where an expression's SourceLoc was set. However, a SourceLoc 
> > is a single character position; I couldn't figure out where an expression's 
> > SourceRange was even set. So I gave up and went down the current approach.
> > 
> > Should we ping the LLVM mailing list and see if this change rings a bell 
> > with anyone there?
> Yes, I believe that changing Clang to produce consistent source ranges in 
> C++14 and C++17 is the best way to go.
> 
> Manual parsing below is not a very maintainable solution.
> Yes, I believe that changing Clang to produce consistent source ranges in 
> C++14 and C++17 is the best way to go.

+1, if this is possible to do.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:67-70
+// With -std c++14 or earlier (!LangOps.CPlusPlus17), it was sufficient to
+// return CtorExpr->getSourceRange(). However, starting with c++17, parsing
+// the expression 'std::string Name = ""' results in a CtorExpr whose
+// SourceRange includes just '""' rather than the previous 'Name = ""'.

poelmanc wrote:
> aaron.ballman wrote:
> > Are you sure that this behavioral difference isn't just a bug in Clang? I 
> > can't think of why the source range should differ based on language mode, 
> > so I wonder if the correct thing to do is fix how we calculate the source 
> > range in Clang?
> Thanks and no, I'm not sure at all. At the end of my summary paragraph I 
> wrote:
> 
> > Or, is it possible that this change in SourceRange is an unintentional 
> > difference in the parsing code? If so fixing that might make more sense.
> 
> Before starting this patch, I built a Debug build of LLVM and stepped through 
> LLVM parsing code while running clang-tidy on the 
> readability-redundant-string-init.cpp file. I set breakpoints and inspected 
> variable values under both -std c++14 and -std c++17. The call stacks were 
> clearly different. I could sometimes inspect character positions in the file 
> and see where an expression's SourceLoc was set. However, a SourceLoc is a 
> single character position; I couldn't figure out where an expression's 
> SourceRange was even set. So I gave up and went down the current approach.
> 
> Should we ping the LLVM mailing list and see if this change rings a bell with 
> anyone there?
Yes, I believe that changing Clang to produce consistent source ranges in C++14 
and C++17 is the best way to go.

Manual parsing below is not a very maintainable solution.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc marked 2 inline comments as done.
poelmanc added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:67-70
+// With -std c++14 or earlier (!LangOps.CPlusPlus17), it was sufficient to
+// return CtorExpr->getSourceRange(). However, starting with c++17, parsing
+// the expression 'std::string Name = ""' results in a CtorExpr whose
+// SourceRange includes just '""' rather than the previous 'Name = ""'.

aaron.ballman wrote:
> Are you sure that this behavioral difference isn't just a bug in Clang? I 
> can't think of why the source range should differ based on language mode, so 
> I wonder if the correct thing to do is fix how we calculate the source range 
> in Clang?
Thanks and no, I'm not sure at all. At the end of my summary paragraph I wrote:

> Or, is it possible that this change in SourceRange is an unintentional 
> difference in the parsing code? If so fixing that might make more sense.

Before starting this patch, I built a Debug build of LLVM and stepped through 
LLVM parsing code while running clang-tidy on the 
readability-redundant-string-init.cpp file. I set breakpoints and inspected 
variable values under both -std c++14 and -std c++17. The call stacks were 
clearly different. I could sometimes inspect character positions in the file 
and see where an expression's SourceLoc was set. However, a SourceLoc is a 
single character position; I couldn't figure out where an expression's 
SourceRange was even set. So I gave up and went down the current approach.

Should we ping the LLVM mailing list and see if this change rings a bell with 
anyone there?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:67-70
+// With -std c++14 or earlier (!LangOps.CPlusPlus17), it was sufficient to
+// return CtorExpr->getSourceRange(). However, starting with c++17, parsing
+// the expression 'std::string Name = ""' results in a CtorExpr whose
+// SourceRange includes just '""' rather than the previous 'Name = ""'.

Are you sure that this behavioral difference isn't just a bug in Clang? I can't 
think of why the source range should differ based on language mode, so I wonder 
if the correct thing to do is fix how we calculate the source range in Clang?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp:106
   DECL_STRING(e, "");
-  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
+  // CHECK-MESSAGES: [[@LINE-1]]:1{{[58]}}: warning: redundant string 
initialization
 

poelmanc wrote:
> aaron.ballman wrote:
> > Why does this need a regex?
> Thanks, I added a comment explaining that the character position of the 
> warning differs slightly between C++11/14 (reports at the 'e') and C++17/2x 
> (reports at the '"'), since the underlying parsing code has changed.
Thank you for the explanation!


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc marked 3 inline comments as done.
poelmanc added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:50
   varDecl(hasType(hasUnqualifiedDesugaredType(recordType(
   
hasDeclaration(cxxRecordDecl(hasName("basic_string")),
   hasInitializer(expr(ignoringImplicit(anyOf(

MyDeveloperDay wrote:
> driveby aside comment (feel free to ignore): wouldn't it be good if 
> "basic_string" here was configurable, this would allow anyone who defines a 
> string class (say like StringRef) which could have this kind of redundant 
> string initialization be able to catch them.
> 
> Couldn't this be then used to find code like `StringRef EnabledPrefixOut = 
> "";`
> 
> ```
> static void ParseMRecip(const Driver , const ArgList ,
> ArgStringList ) {
>   StringRef DisabledPrefixIn = "!";
>   StringRef DisabledPrefixOut = "!";
>   StringRef EnabledPrefixOut = "";
>   StringRef Out = "-mrecip=";
> ```
> 
Great suggestion, done. See https://reviews.llvm.org/D69548. (I figured it 
should be a separate topic from this one, but holler if you prefer I add it to 
this patch.)


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:50
   varDecl(hasType(hasUnqualifiedDesugaredType(recordType(
   
hasDeclaration(cxxRecordDecl(hasName("basic_string")),
   hasInitializer(expr(ignoringImplicit(anyOf(

driveby aside comment (feel free to ignore): wouldn't it be good if 
"basic_string" here was configurable, this would allow anyone who defines a 
string class (say like StringRef) which could have this kind of redundant 
string initialization be able to catch them.

Couldn't this be then used to find code like `StringRef EnabledPrefixOut = "";`

```
static void ParseMRecip(const Driver , const ArgList ,
ArgStringList ) {
  StringRef DisabledPrefixIn = "!";
  StringRef DisabledPrefixOut = "!";
  StringRef EnabledPrefixOut = "";
  StringRef Out = "-mrecip=";
```



Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc marked 5 inline comments as done.
poelmanc added a comment.

Thanks for the feedback, the new patch removes the extra `const`, `clang::`, 
and braces on one-line `if` statements, and now explains the regex in 
readability-redundant-string-init.cpp.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp:106
   DECL_STRING(e, "");
-  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
+  // CHECK-MESSAGES: [[@LINE-1]]:1{{[58]}}: warning: redundant string 
initialization
 

aaron.ballman wrote:
> Why does this need a regex?
Thanks, I added a comment explaining that the character position of the warning 
differs slightly between C++11/14 (reports at the 'e') and C++17/2x (reports at 
the '"'), since the underlying parsing code has changed.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 226695.
poelmanc added a comment.

Remove extra `const`, braces for one-line `if` statements, and `clang::`. Added 
a comment explaining the need for a regex on a warning test line.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238

Files:
  clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
@@ -104,7 +103,8 @@
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: redundant string initialization
   // CHECK-FIXES: STRING d;
   DECL_STRING(e, "");
-  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
+  // Warning at position :15 with -std=c++11/14, position :18 for -std=c++17/2x
+  // CHECK-MESSAGES: [[@LINE-2]]:1{{[58]}}: warning: redundant string initialization
 
   MyString f = "u";
   STRING g = "u";
Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
Index: clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
@@ -57,12 +57,73 @@
   this);
 }
 
+// Returns the SourceRange for the string constructor expression and a bool
+// indicating whether to fix it (by replacing it with just the variable name)
+// or just issue a diagnostic warning. CtorExpr's SourceRange includes:
+//   foo = ""
+//   bar("")
+//   ""(only in C++17 and later)
+//
+// With -std c++14 or earlier (!LangOps.CPlusPlus17), it was sufficient to
+// return CtorExpr->getSourceRange(). However, starting with c++17, parsing
+// the expression 'std::string Name = ""' results in a CtorExpr whose
+// SourceRange includes just '""' rather than the previous 'Name = ""'.
+//
+// So if the CtorExpr's SourceRange doesn't already start with 'Name', we
+// search leftward for 'Name\b*=\b*' (where \b* represents optional whitespace)
+// and if found, extend the SourceRange to start at 'Name'.
+std::pair
+CtorSourceRange(const MatchFinder::MatchResult , const NamedDecl *Decl,
+const Expr *CtorExpr) {
+  const SourceManager  = *Result.SourceManager;
+  StringRef Name = Decl->getName();
+  int NameLength = Name.size();
+  SourceLocation CtorStartLoc(CtorExpr->getSourceRange().getBegin());
+  std::pair CtorLocInfo = SM.getDecomposedLoc(
+  CtorStartLoc.isMacroID() ? SM.getImmediateMacroCallerLoc(CtorStartLoc)
+   : CtorStartLoc);
+  int CtorStartPos = CtorLocInfo.second;
+  StringRef File = SM.getBufferData(CtorLocInfo.first);
+  StringRef CtorCheckName = File.substr(CtorStartPos, NameLength);
+  if (CtorCheckName == Name)
+// For 'std::string foo("");', CtorExpr is 'foo("")'
+return std::make_pair(CtorExpr->getSourceRange(), true);
+  // Scan backwards from CtorStartPos.
+  // If find "Name\b*=\b*", extend CtorExpr SourceRange leftwards to include it.
+  bool FoundEquals = false;
+  for (int Pos = CtorStartPos - 1; Pos >= 0; Pos--) {
+char Char = File.data()[Pos];
+if (Char == '=') {
+  if (FoundEquals)
+break; // Only allow one equals sign
+  FoundEquals = true;
+} else if (!isWhitespace(Char)) {
+  // First non-whitespace/non-equals char. Check for Name ending with it.
+  int CheckNameStart = Pos - NameLength + 1;
+  StringRef CheckName(File.data() + CheckNameStart, NameLength);
+  if (FoundEquals && Name == CheckName) {
+return std::make_pair(SourceRange(CtorStartLoc.getLocWithOffset(
+  CheckNameStart - CtorStartPos),
+

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:75
+// and if found, extend the SourceRange to start at 'Name'.
+std::pair
+CtorSourceRange(const MatchFinder::MatchResult ,

You can drop the `clang::` everywhere -- the code is within the correct 
namespace already.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:79-80
+  const SourceManager  = *Result.SourceManager;
+  const StringRef Name = Decl->getName();
+  const int NameLength = Name.size();
+  const SourceLocation CtorStartLoc(CtorExpr->getSourceRange().getBegin());

Please drop top-level `const` qualifiers unless it's a pointer or reference 
declaration (that's not a style we typically use).



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:128
+  diag(CtorExprRange.first.getBegin(), "redundant string initialization");
+  if (CtorExprRange.second) {
+Diag << FixItHint::CreateReplacement(CtorExprRange.first, Decl->getName());

Elide braces per coding style.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp:106
   DECL_STRING(e, "");
-  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
+  // CHECK-MESSAGES: [[@LINE-1]]:1{{[58]}}: warning: redundant string 
initialization
 

Why does this need a regex?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc marked 2 inline comments as done.
poelmanc added a comment.

Thanks for the quick feedback, fixed.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 225894.
poelmanc added a comment.

Removed the two uses of auto where the type was not an iterator or clear from 
the right-hand-side.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238

Files:
  clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
@@ -104,7 +103,7 @@
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: redundant string initialization
   // CHECK-FIXES: STRING d;
   DECL_STRING(e, "");
-  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
+  // CHECK-MESSAGES: [[@LINE-1]]:1{{[58]}}: warning: redundant string initialization
 
   MyString f = "u";
   STRING g = "u";
Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
Index: clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
@@ -57,12 +57,77 @@
   this);
 }
 
+// Returns the SourceRange for the string constructor expression and a bool
+// indicating whether to fix it (by replacing it with just the variable name)
+// or just issue a diagnostic warning. CtorExpr's SourceRange includes:
+//   foo = ""
+//   bar("")
+//   ""(only in C++17 and later)
+//
+// With -std c++14 or earlier (!LangOps.CPlusPlus17), it was sufficient to
+// return CtorExpr->getSourceRange(). However, starting with c++17, parsing
+// the expression 'std::string Name = ""' results in a CtorExpr whose
+// SourceRange includes just '""' rather than the previous 'Name = ""'.
+//
+// So if the CtorExpr's SourceRange doesn't already start with 'Name', we
+// search leftward for 'Name\b*=\b*' (where \b* represents optional whitespace)
+// and if found, extend the SourceRange to start at 'Name'.
+std::pair
+CtorSourceRange(const MatchFinder::MatchResult ,
+const clang::NamedDecl *Decl, const clang::Expr *CtorExpr) {
+  const SourceManager  = *Result.SourceManager;
+  const StringRef Name = Decl->getName();
+  const int NameLength = Name.size();
+  const SourceLocation CtorStartLoc(CtorExpr->getSourceRange().getBegin());
+  const std::pair CtorLocInfo = SM.getDecomposedLoc(
+  CtorStartLoc.isMacroID() ? SM.getImmediateMacroCallerLoc(CtorStartLoc)
+   : CtorStartLoc);
+  const int CtorStartPos = CtorLocInfo.second;
+  const StringRef File = SM.getBufferData(CtorLocInfo.first);
+  const StringRef CtorCheckName = File.substr(CtorStartPos, NameLength);
+  if (CtorCheckName == Name) {
+// For 'std::string foo("");', CtorExpr is 'foo("")'
+return std::make_pair(CtorExpr->getSourceRange(), true);
+  }
+  // Scan backwards from CtorStartPos.
+  // If find "Name\b*=\b*", extend CtorExpr SourceRange leftwards to include it.
+  bool FoundEquals = false;
+  for (int Pos = CtorStartPos - 1; Pos >= 0; Pos--) {
+const char Char = File.data()[Pos];
+if (Char == '=') {
+  if (FoundEquals) {
+break; // Only allow one equals sign
+  }
+  FoundEquals = true;
+} else if (!isWhitespace(Char)) {
+  // First non-whitespace/non-equals char. Check for Name ending with it.
+  const int CheckNameStart = Pos - NameLength + 1;
+  const StringRef CheckName(File.data() + CheckNameStart, NameLength);
+  if (FoundEquals && Name == CheckName) {
+return std::make_pair(
+clang::SourceRange(
+CtorStartLoc.getLocWithOffset(CheckNameStart - CtorStartPos),
+CtorExpr->getSourceRange().getEnd()),
+

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:78
+const clang::NamedDecl *Decl, const clang::Expr *CtorExpr) {
+  const auto  = *Result.SourceManager;
+  const StringRef Name = Decl->getName();

Please do not use auto if type is not spelled in same statement or iterator



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:124
   const auto *Decl = Result.Nodes.getNodeAs("decl");
-  diag(CtorExpr->getExprLoc(), "redundant string initialization")
-  << FixItHint::CreateReplacement(CtorExpr->getSourceRange(),
-  Decl->getName());
+  const auto CtorExprRange = CtorSourceRange(Result, Decl, CtorExpr);
+  DiagnosticBuilder Diag =

Please do not use auto if type is not spelled in same statement or iterator


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69238/new/

https://reviews.llvm.org/D69238



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

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc created this revision.
poelmanc added reviewers: gribozavr, etienneb, alexfh.
Herald added subscribers: cfe-commits, mgehre, dylanmckay.
Herald added a project: clang.

`readability-redundant-string-init` was one of several clang-tidy checks 
documented as failing for C++17 by @gribozavr's extensive testing on 
2019-05-20. (The failure mode in C++17 is that it changes `std::string Name = 
"";` to `std::string Name = Name;`, which actually compiles.)

I found that the `CtorExpr->getSourceRange()` for `string Name = ""` changed 
from spanning the `Name = ""` in C++11/14 to only spanning the `""` in 
C++17/2a. My fix was to add a `CtorSourceRange` function that, if CtorExpr does 
not already start with `Name`, searches backwards for `Name = ` and extends the 
SourceRange to include it. The readability-redundant-string-init.cpp and 
readability-redundant-string-init-msvc.cpp tests now pass for C++11/14/17/2a. 
(The warning for `DECL_STRING(e, "");` is reported at different character 
positions in 11/14 versus 17/2a, so I updated that test to accept either 
position.)

As always I welcome any feedback or suggestions for improvement. In particular, 
the fix would be far simpler if there's some other expression matcher that 
captures the whole `Name = ""` expression. Or, is it possible that this change 
in SourceRange is an unintentional difference in the parsing code? If so fixing 
that might make more sense.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D69238

Files:
  clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
@@ -104,7 +103,7 @@
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: redundant string initialization
   // CHECK-FIXES: STRING d;
   DECL_STRING(e, "");
-  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
+  // CHECK-MESSAGES: [[@LINE-1]]:1{{[58]}}: warning: redundant string initialization
 
   MyString f = "u";
   STRING g = "u";
Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
Index: clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
@@ -57,12 +57,76 @@
   this);
 }
 
+// Returns the SourceRange for the string constructor expression and a bool
+// indicating whether to fix it (by replacing it with just the variable name)
+// or just issue a diagnostic warning. CtorExpr's SourceRange includes:
+//   foo = ""
+//   bar("")
+//   ""(only in C++17 and later)
+//
+// With -std c++14 or earlier (!LangOps.CPlusPlus17), it was sufficient to
+// return CtorExpr->getSourceRange(). However, starting with c++17, parsing
+// the expression 'std::string Name = ""' results in a CtorExpr whose
+// SourceRange includes just '""' rather than the previous 'Name = ""'.
+//
+// So if the CtorExpr's SourceRange doesn't already start with 'Name', we
+// search leftward for 'Name\b*=\b*' (where \b* represents optional whitespace)
+// and if found, extend the SourceRange to start at 'Name'.
+std::pair
+CtorSourceRange(const MatchFinder::MatchResult ,
+const clang::NamedDecl *Decl, const clang::Expr *CtorExpr) {
+  const auto  = *Result.SourceManager;
+  const StringRef Name = Decl->getName();
+  const int NameLength = Name.size();
+  const SourceLocation CtorStartLoc(CtorExpr->getSourceRange().getBegin());
+  const std::pair CtorLocInfo = SM.getDecomposedLoc(
+  CtorStartLoc.isMacroID() ? SM.getImmediateMacroCallerLoc(CtorStartLoc)
+   : CtorStartLoc);
+  const int CtorStartPos = CtorLocInfo.second;