[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

@carlosgalvezp Thanks for your review! I committed it myself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138655

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

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGce6de98b80ad: [clang-tidy] Fix 
`cppcoreguidelines-init-variables` for invalid vardecl (authored by Sockke).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138655

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -- -- 
-fno-delayed-template-parsing -fexceptions
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables -fix-errors %t 
-- -- -fno-delayed-template-parsing -fexceptions
 // CHECK-FIXES: {{^}}#include 
 
 // Ensure that function declarations are not changed.
@@ -124,3 +124,13 @@
   Fruit fruit;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'fruit' is not 
initialized [cppcoreguidelines-init-variables]
 }
+
+void test_clang_diagnostic_error() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized 
[cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+
+  UnknownType b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: error: unknown type name 'UnknownType' 
[clang-diagnostic-error]
+  // CHECK-FIXES-NOT: {{^}}  UnknownType b = 0;{{$}}
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -59,6 +59,10 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  // Clang diagnostic error may cause the variable to be an invalid int vardecl
+  if (MatchedDecl->isInvalidDecl())
+return;
+
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -- -- -fno-delayed-template-parsing -fexceptions
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables -fix-errors %t -- -- -fno-delayed-template-parsing -fexceptions
 // CHECK-FIXES: {{^}}#include 
 
 // Ensure that function declarations are not changed.
@@ -124,3 +124,13 @@
   Fruit fruit;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'fruit' is not initialized [cppcoreguidelines-init-variables]
 }
+
+void test_clang_diagnostic_error() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized [cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+
+  UnknownType b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: error: unknown type name 'UnknownType' [clang-diagnostic-error]
+  // CHECK-FIXES-NOT: {{^}}  UnknownType b = 0;{{$}}
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -59,6 +59,10 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  // Clang diagnostic error may cause the variable to be an invalid int vardecl
+  if (MatchedDecl->isInvalidDecl())
+return;
+
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 495413.
Sockke added a comment.

Rebased.


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

https://reviews.llvm.org/D138655

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -- -- 
-fno-delayed-template-parsing -fexceptions
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables -fix-errors %t 
-- -- -fno-delayed-template-parsing -fexceptions
 // CHECK-FIXES: {{^}}#include 
 
 // Ensure that function declarations are not changed.
@@ -124,3 +124,13 @@
   Fruit fruit;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'fruit' is not 
initialized [cppcoreguidelines-init-variables]
 }
+
+void test_clang_diagnostic_error() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized 
[cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+
+  UnknownType b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: error: unknown type name 'UnknownType' 
[clang-diagnostic-error]
+  // CHECK-FIXES-NOT: {{^}}  UnknownType b = 0;{{$}}
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -59,6 +59,10 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  // Clang diagnostic error may cause the variable to be an invalid int vardecl
+  if (MatchedDecl->isInvalidDecl())
+return;
+
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -- -- -fno-delayed-template-parsing -fexceptions
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables -fix-errors %t -- -- -fno-delayed-template-parsing -fexceptions
 // CHECK-FIXES: {{^}}#include 
 
 // Ensure that function declarations are not changed.
@@ -124,3 +124,13 @@
   Fruit fruit;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'fruit' is not initialized [cppcoreguidelines-init-variables]
 }
+
+void test_clang_diagnostic_error() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized [cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+
+  UnknownType b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: error: unknown type name 'UnknownType' [clang-diagnostic-error]
+  // CHECK-FIXES-NOT: {{^}}  UnknownType b = 0;{{$}}
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -59,6 +59,10 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  // Clang diagnostic error may cause the variable to be an invalid int vardecl
+  if (MatchedDecl->isInvalidDecl())
+return;
+
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Hi, Could anyone please review this diff?


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

https://reviews.llvm.org/D138655

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

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping.


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

https://reviews.llvm.org/D138655

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

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping.


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

https://reviews.llvm.org/D138655

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

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke marked 6 inline comments as done.
Sockke added a comment.

Mark comments.


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

https://reviews.llvm.org/D138655

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

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 484765.
Sockke added a comment.

Improved the test file.


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

https://reviews.llvm.org/D138655

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -- -- 
-fno-delayed-template-parsing -fexceptions
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables -fix-errors %t 
-- -- -fno-delayed-template-parsing -fexceptions
 // CHECK-FIXES: {{^}}#include 
 
 // Ensure that function declarations are not changed.
@@ -124,3 +124,13 @@
   Fruit fruit;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'fruit' is not 
initialized [cppcoreguidelines-init-variables]
 }
+
+void test_clang_diagnostic_error() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized 
[cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+
+  UnknownType b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: error: unknown type name 'UnknownType' 
[clang-diagnostic-error]
+  // CHECK-FIXES-NOT: {{^}}  UnknownType b = 0;{{$}}
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -60,6 +60,10 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  // Clang diagnostic error may cause the variable to be an invalid int vardecl
+  if (MatchedDecl->isInvalidDecl())
+return;
+
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -- -- -fno-delayed-template-parsing -fexceptions
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables -fix-errors %t -- -- -fno-delayed-template-parsing -fexceptions
 // CHECK-FIXES: {{^}}#include 
 
 // Ensure that function declarations are not changed.
@@ -124,3 +124,13 @@
   Fruit fruit;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'fruit' is not initialized [cppcoreguidelines-init-variables]
 }
+
+void test_clang_diagnostic_error() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized [cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+
+  UnknownType b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: error: unknown type name 'UnknownType' [clang-diagnostic-error]
+  // CHECK-FIXES-NOT: {{^}}  UnknownType b = 0;{{$}}
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -60,6 +60,10 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  // Clang diagnostic error may cause the variable to be an invalid int vardecl
+  if (MatchedDecl->isInvalidDecl())
+return;
+
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 484210.
Sockke added a comment.

Added comments


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

https://reviews.llvm.org/D138655

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
@@ -1,6 +1,16 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -- -- 
-fno-delayed-template-parsing -fexceptions
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables -fix-errors %t 
-- -- -fno-delayed-template-parsing -fexceptions
 // CHECK-FIXES: {{^}}#include 
 
+// Header file error causes stl container variable to be invalid int vardecl
+#include "unknown.h" 
+// CHECK-MESSAGES: :[[@LINE-1]]:10: error: 'unknown.h' file not found 
[clang-diagnostic-error]
+
+namespace std {
+template 
+struct vector {
+  vector();
+};
+
 // Ensure that function declarations are not changed.
 void some_func(int x, double d, bool b, const char *p);
 
@@ -124,3 +134,13 @@
   Fruit fruit;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'fruit' is not 
initialized [cppcoreguidelines-init-variables]
 }
+
+void test_clang_diagnostic_error() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized 
[cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+
+  // The stl object has been initialized
+  std::vector arr;
+  // CHECK-FIXES-NOT: {{^}}  std::vector arr = 0;{{$}}
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -60,6 +60,9 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  // Header file error causes the stl container variable to be an invalid int 
vardecl
+  if (MatchedDecl->isInvalidDecl())
+return;
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
@@ -1,6 +1,16 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -- -- -fno-delayed-template-parsing -fexceptions
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables -fix-errors %t -- -- -fno-delayed-template-parsing -fexceptions
 // CHECK-FIXES: {{^}}#include 
 
+// Header file error causes stl container variable to be invalid int vardecl
+#include "unknown.h" 
+// CHECK-MESSAGES: :[[@LINE-1]]:10: error: 'unknown.h' file not found [clang-diagnostic-error]
+
+namespace std {
+template 
+struct vector {
+  vector();
+};
+
 // Ensure that function declarations are not changed.
 void some_func(int x, double d, bool b, const char *p);
 
@@ -124,3 +134,13 @@
   Fruit fruit;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'fruit' is not initialized [cppcoreguidelines-init-variables]
 }
+
+void test_clang_diagnostic_error() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized [cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+
+  // The stl object has been initialized
+  std::vector arr;
+  // CHECK-FIXES-NOT: {{^}}  std::vector arr = 0;{{$}}
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -60,6 +60,9 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  // Header file error causes the stl container variable to be an invalid int vardecl
+  if (MatchedDecl->isInvalidDecl())
+return;
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping.


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

https://reviews.llvm.org/D138655

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

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping.


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

https://reviews.llvm.org/D138655

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

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 478529.
Sockke added a comment.

Added test in existing `init-variables.cpp` file.


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

https://reviews.llvm.org/D138655

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
@@ -1,6 +1,15 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -- -- 
-fno-delayed-template-parsing -fexceptions
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables -fix-errors %t 
-- -- -fno-delayed-template-parsing -fexceptions
 // CHECK-FIXES: {{^}}#include 
 
+#include "unknown.h"
+// CHECK-MESSAGES: :[[@LINE-1]]:10: error: 'unknown.h' file not found 
[clang-diagnostic-error]
+
+namespace std {
+template 
+struct vector {
+  vector();
+};
+
 // Ensure that function declarations are not changed.
 void some_func(int x, double d, bool b, const char *p);
 
@@ -124,3 +133,10 @@
   Fruit fruit;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'fruit' is not 
initialized [cppcoreguidelines-init-variables]
 }
+
+void test_clang_diagnostic_error() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized 
[cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+  std::vector arr;
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -60,6 +60,8 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  if (MatchedDecl->isInvalidDecl())
+return;
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
@@ -1,6 +1,15 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -- -- -fno-delayed-template-parsing -fexceptions
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables -fix-errors %t -- -- -fno-delayed-template-parsing -fexceptions
 // CHECK-FIXES: {{^}}#include 
 
+#include "unknown.h"
+// CHECK-MESSAGES: :[[@LINE-1]]:10: error: 'unknown.h' file not found [clang-diagnostic-error]
+
+namespace std {
+template 
+struct vector {
+  vector();
+};
+
 // Ensure that function declarations are not changed.
 void some_func(int x, double d, bool b, const char *p);
 
@@ -124,3 +133,10 @@
   Fruit fruit;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'fruit' is not initialized [cppcoreguidelines-init-variables]
 }
+
+void test_clang_diagnostic_error() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized [cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+  std::vector arr;
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -60,6 +60,8 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  if (MatchedDecl->isInvalidDecl())
+return;
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 477861.
Sockke added a comment.

Added test.


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

https://reviews.llvm.org/D138655

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables-clang-diagnostic-error.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables-clang-diagnostic-error.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables-clang-diagnostic-error.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -fix-errors --
+
+#include "unknown.h"
+// CHECK-MESSAGES: :[[@LINE-1]]:10: error: 'unknown.h' file not found 
[clang-diagnostic-error]
+
+namespace std {
+template 
+struct vector {
+  vector();
+};
+
+void test() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized 
[cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+  std::vector arr;
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -60,6 +60,8 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  if (MatchedDecl->isInvalidDecl())
+return;
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables-clang-diagnostic-error.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables-clang-diagnostic-error.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -fix-errors --
+
+#include "unknown.h"
+// CHECK-MESSAGES: :[[@LINE-1]]:10: error: 'unknown.h' file not found [clang-diagnostic-error]
+
+namespace std {
+template 
+struct vector {
+  vector();
+};
+
+void test() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized [cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+  std::vector arr;
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -60,6 +60,8 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  if (MatchedDecl->isInvalidDecl())
+return;
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[gehry via Phabricator via cfe-commits]

Sockke created this revision.
Sockke added a reviewer: aaron.ballman.
Herald added subscribers: carlosgalvezp, shchenz, kbarton, xazax.hun, nemanjai.
Herald added a reviewer: njames93.
Herald added a project: All.
Sockke requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

https://godbolt.org/z/n4cK4fo3o
The checker missed a check for invalid vardecl and this could cause a false 
positive.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138655

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp


Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -60,6 +60,8 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  if (MatchedDecl->isInvalidDecl())
+return;
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //


Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -60,6 +60,8 @@
   const ASTContext  = *Result.Context;
   const SourceManager  = Context.getSourceManager();
 
+  if (MatchedDecl->isInvalidDecl())
+return;
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

This patch has been around for a long time, I have added the release notes. 
There are users waiting for this fix and I committed it myself first.
@LegalizeAdulthood I'm looking forward to your follow-up reply.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127293

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

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2d8e91a5d359: [clang-tidy] Ignore other members in a union 
if any member of it is initialized… (authored by Sockke).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127293

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
@@ -552,3 +552,30 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  union {
+int a;
+int b;
+  };
+};
+
+struct S3 {
+  S3() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A [cppcoreguidelines-pro-type-member-init]
+  int A;
+  // CHECK-FIXES: int A{};
+  union {
+int B;
+int C = 0;
+  };
+};
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -120,6 +120,11 @@
   ` check. Partial
   support for C++14 signal handler rules was added. Bug report generation was
   improved.
+
+- Fixed a false positive in :doc:`cppcoreguidelines-pro-type-member-init
+  ` when warnings
+  would be emitted for uninitialized members of an anonymous union despite
+  there being an initializer for one of the other members.
   
 - Improved `modernize-use-emplace `_ check.
 
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -61,6 +61,17 @@
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+SmallPtrSetImpl ) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+// Erase all members in a union if any member of it is initialized.
+for (const auto *F : R->fields())
+  FieldDecls.erase(F);
+  } else
+FieldDecls.erase(M);
+}
+
 void removeFieldsInitializedInBody(
 const Stmt , ASTContext ,
 SmallPtrSetImpl ) {
@@ -70,7 +81,7 @@
 hasLHS(memberExpr(member(fieldDecl().bind("fieldDecl")),
 Stmt, Context);
   for (const auto  : Matches)
-FieldDecls.erase(Match.getNodeAs("fieldDecl"));
+removeFieldInitialized(Match.getNodeAs("fieldDecl"), FieldDecls);
 }
 
 StringRef getName(const FieldDecl *Field) { return Field->getName(); }
@@ -418,13 +429,20 @@
 
   // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet FieldsToInit;
-  forEachField(ClassDecl, ClassDecl.fields(), [&](const FieldDecl *F) {
+  bool AnyMemberHasInitPerUnion = false;
+  forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (IgnoreArrays && F->getType()->isArrayType())
   return;
+if (F->hasInClassInitializer() && F->getParent()->isUnion()) {
+  AnyMemberHasInitPerUnion = true;
+  removeFieldInitialized(F, FieldsToInit);
+}
 if (!F->hasInClassInitializer() &&
 utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
 Context) &&
-!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield())
+!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield() &&
+!AnyMemberHasInitPerUnion)
   FieldsToInit.insert(F);
   });
   if (FieldsToInit.empty())
@@ -437,7 +455,7 @@
   if (Init->isAnyMemberInitializer() && Init->isWritten()) {
 if (IsUnion)
   return; // We can only initialize one member of a union.
-FieldsToInit.erase(Init->getAnyMember());
+removeFieldInitialized(Init->getAnyMember(), FieldsToInit);
   }
 }
 removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
@@ -478,7 +496,7 @@
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet FieldsToFix;
-  bool AnyMemberHasInitPerUnion = false;
+  AnyMemberHasInitPerUnion = false;
   

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 455516.
Sockke added a comment.

Rebased.


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

https://reviews.llvm.org/D127293

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
@@ -552,3 +552,30 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  union {
+int a;
+int b;
+  };
+};
+
+struct S3 {
+  S3() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A [cppcoreguidelines-pro-type-member-init]
+  int A;
+  // CHECK-FIXES: int A{};
+  union {
+int B;
+int C = 0;
+  };
+};
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -120,6 +120,11 @@
   ` check. Partial
   support for C++14 signal handler rules was added. Bug report generation was
   improved.
+
+- Fixed a false positive in :doc:`cppcoreguidelines-pro-type-member-init
+  ` when warnings
+  would be emitted for uninitialized members of an anonymous union despite
+  there being an initializer for one of the other members.
   
 - Improved `modernize-use-emplace `_ check.
 
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -61,6 +61,17 @@
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+SmallPtrSetImpl ) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+// Erase all members in a union if any member of it is initialized.
+for (const auto *F : R->fields())
+  FieldDecls.erase(F);
+  } else
+FieldDecls.erase(M);
+}
+
 void removeFieldsInitializedInBody(
 const Stmt , ASTContext ,
 SmallPtrSetImpl ) {
@@ -70,7 +81,7 @@
 hasLHS(memberExpr(member(fieldDecl().bind("fieldDecl")),
 Stmt, Context);
   for (const auto  : Matches)
-FieldDecls.erase(Match.getNodeAs("fieldDecl"));
+removeFieldInitialized(Match.getNodeAs("fieldDecl"), FieldDecls);
 }
 
 StringRef getName(const FieldDecl *Field) { return Field->getName(); }
@@ -418,13 +429,20 @@
 
   // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet FieldsToInit;
-  forEachField(ClassDecl, ClassDecl.fields(), [&](const FieldDecl *F) {
+  bool AnyMemberHasInitPerUnion = false;
+  forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (IgnoreArrays && F->getType()->isArrayType())
   return;
+if (F->hasInClassInitializer() && F->getParent()->isUnion()) {
+  AnyMemberHasInitPerUnion = true;
+  removeFieldInitialized(F, FieldsToInit);
+}
 if (!F->hasInClassInitializer() &&
 utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
 Context) &&
-!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield())
+!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield() &&
+!AnyMemberHasInitPerUnion)
   FieldsToInit.insert(F);
   });
   if (FieldsToInit.empty())
@@ -437,7 +455,7 @@
   if (Init->isAnyMemberInitializer() && Init->isWritten()) {
 if (IsUnion)
   return; // We can only initialize one member of a union.
-FieldsToInit.erase(Init->getAnyMember());
+removeFieldInitialized(Init->getAnyMember(), FieldsToInit);
   }
 }
 removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
@@ -478,7 +496,7 @@
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet FieldsToFix;
-  bool AnyMemberHasInitPerUnion = false;
+  AnyMemberHasInitPerUnion = false;
   forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
  AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping @LegalizeAdulthood.


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

https://reviews.llvm.org/D127293

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

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Hi, @LegalizeAdulthood, are you okay with the way this has progressed?


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

https://reviews.llvm.org/D127293

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

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 449188.
Sockke added a comment.

Rebased.


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

https://reviews.llvm.org/D127293

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
@@ -552,3 +552,30 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  union {
+int a;
+int b;
+  };
+};
+
+struct S3 {
+  S3() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A [cppcoreguidelines-pro-type-member-init]
+  int A;
+  // CHECK-FIXES: int A{};
+  union {
+int B;
+int C = 0;
+  };
+};
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -105,6 +105,11 @@
 Changes in existing checks
 ^^
 
+- Fixed a false positive in :doc:`cppcoreguidelines-pro-type-member-init
+  ` when warnings
+  would be emitted for uninitialized members of an anonymous union despite
+  there being an initializer for one of the other members.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -61,6 +61,17 @@
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+SmallPtrSetImpl ) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+// Erase all members in a union if any member of it is initialized.
+for (const auto *F : R->fields())
+  FieldDecls.erase(F);
+  } else
+FieldDecls.erase(M);
+}
+
 void removeFieldsInitializedInBody(
 const Stmt , ASTContext ,
 SmallPtrSetImpl ) {
@@ -70,7 +81,7 @@
 hasLHS(memberExpr(member(fieldDecl().bind("fieldDecl")),
 Stmt, Context);
   for (const auto  : Matches)
-FieldDecls.erase(Match.getNodeAs("fieldDecl"));
+removeFieldInitialized(Match.getNodeAs("fieldDecl"), FieldDecls);
 }
 
 StringRef getName(const FieldDecl *Field) { return Field->getName(); }
@@ -418,13 +429,20 @@
 
   // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet FieldsToInit;
-  forEachField(ClassDecl, ClassDecl.fields(), [&](const FieldDecl *F) {
+  bool AnyMemberHasInitPerUnion = false;
+  forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (IgnoreArrays && F->getType()->isArrayType())
   return;
+if (F->hasInClassInitializer() && F->getParent()->isUnion()) {
+  AnyMemberHasInitPerUnion = true;
+  removeFieldInitialized(F, FieldsToInit);
+}
 if (!F->hasInClassInitializer() &&
 utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
 Context) &&
-!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield())
+!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield() &&
+!AnyMemberHasInitPerUnion)
   FieldsToInit.insert(F);
   });
   if (FieldsToInit.empty())
@@ -437,7 +455,7 @@
   if (Init->isAnyMemberInitializer() && Init->isWritten()) {
 if (IsUnion)
   return; // We can only initialize one member of a union.
-FieldsToInit.erase(Init->getAnyMember());
+removeFieldInitialized(Init->getAnyMember(), FieldsToInit);
   }
 }
 removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
@@ -478,7 +496,7 @@
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet FieldsToFix;
-  bool AnyMemberHasInitPerUnion = false;
+  AnyMemberHasInitPerUnion = false;
   forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
  AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128697: [clang-tidy] Add new check `bugprone-unhandled-exception-at-sto`

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128697

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

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping.


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

https://reviews.llvm.org/D127293

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

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 445792.
Sockke added a comment.

Added test cases where there is a struct with an anonymous union and another 
field that does need an initializer.


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

https://reviews.llvm.org/D127293

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
@@ -552,3 +552,30 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  union {
+int a;
+int b;
+  };
+};
+
+struct S3 {
+  S3() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A [cppcoreguidelines-pro-type-member-init]
+  int A;
+  // CHECK-FIXES: int A{};
+  union {
+int B;
+int C = 0;
+  };
+};
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -201,6 +201,11 @@
   Fixed an issue when there was already an initializer in the constructor and
   the check would try to create another initializer for the same member.
 
+- Fixed a false positive in :doc:`cppcoreguidelines-pro-type-member-init
+  ` when warnings
+  would be emitted for uninitialized members of an anonymous union despite
+  there being an initializer for one of the other members.
+
 - Fixed a false positive in :doc:`cppcoreguidelines-virtual-class-destructor
   ` involving
   ``final`` classes. The check will not diagnose classes marked ``final``, since
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -61,6 +61,17 @@
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+SmallPtrSetImpl ) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+// Erase all members in a union if any member of it is initialized.
+for (const auto *F : R->fields())
+  FieldDecls.erase(F);
+  } else
+FieldDecls.erase(M);
+}
+
 void removeFieldsInitializedInBody(
 const Stmt , ASTContext ,
 SmallPtrSetImpl ) {
@@ -70,7 +81,7 @@
 hasLHS(memberExpr(member(fieldDecl().bind("fieldDecl")),
 Stmt, Context);
   for (const auto  : Matches)
-FieldDecls.erase(Match.getNodeAs("fieldDecl"));
+removeFieldInitialized(Match.getNodeAs("fieldDecl"), FieldDecls);
 }
 
 StringRef getName(const FieldDecl *Field) { return Field->getName(); }
@@ -418,13 +429,20 @@
 
   // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet FieldsToInit;
-  forEachField(ClassDecl, ClassDecl.fields(), [&](const FieldDecl *F) {
+  bool AnyMemberHasInitPerUnion = false;
+  forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (IgnoreArrays && F->getType()->isArrayType())
   return;
+if (F->hasInClassInitializer() && F->getParent()->isUnion()) {
+  AnyMemberHasInitPerUnion = true;
+  removeFieldInitialized(F, FieldsToInit);
+}
 if (!F->hasInClassInitializer() &&
 utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
 Context) &&
-!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield())
+!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield() &&
+!AnyMemberHasInitPerUnion)
   FieldsToInit.insert(F);
   });
   if (FieldsToInit.empty())
@@ -437,7 +455,7 @@
   if (Init->isAnyMemberInitializer() && Init->isWritten()) {
 if (IsUnion)
   return; // We can only initialize one member of a union.
-FieldsToInit.erase(Init->getAnyMember());
+removeFieldInitialized(Init->getAnyMember(), FieldsToInit);
   }
 }
 removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
@@ -478,7 +496,7 @@
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet FieldsToFix;
-  bool AnyMemberHasInitPerUnion = false;
+  AnyMemberHasInitPerUnion = false;
   forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
  

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping.


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

https://reviews.llvm.org/D127293

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

[PATCH] D128697: [clang-tidy] Add new check `bugprone-unhandled-exception-at-sto`

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128697

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

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping.


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

https://reviews.llvm.org/D127293

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

[PATCH] D128697: [clang-tidy] Add new check `bugprone-unhandled-exception-at-sto`

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

In D128697#3619310 , 
@LegalizeAdulthood wrote:

> This whole check seems weird to me.  I mean, almost every use of a standard 
> container could throw `std::bad_alloc` but we don't insist on a local `catch` 
> for `bad_alloc` at every possible throwing call site.
>
> Why would we assume that every call site of `stoi` or `stod` **must** have an 
> exception handler immediately around it?  It's perfectly acceptable for an 
> application to handle this at an outer scope that can't be detected by 
> clang-tidy.

Makes sense, I implemented this check here because some projects in ByteDance 
used stoi with missing exception handlers caused an online crash, I think this 
is a relatively common problem.  Perhaps only report diagnostics for stoi calls 
in nothrow functions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128697

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

[PATCH] D128697: [clang-tidy] Add new check `bugprone-unhandled-exception-at-sto`

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

In D128697#3615404 , @Eugene.Zelenko 
wrote:

> I think will be good idea to make check more generic and allow user-defined 
> list of unsafe functions.

I understand that user-defined is implemented through the config, but some 
types of information of user customization cannot be well described here. Maybe 
use function name and argument counts only to match unsafe functions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128697

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

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 441241.
Sockke added a comment.

Added release notes.


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

https://reviews.llvm.org/D127293

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
@@ -552,3 +552,19 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  union {
+int a;
+int b;
+  };
+};
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -183,6 +183,10 @@
   Fixed an issue when there was already an initializer in the constructor and
   the check would try to create another initializer for the same member.
 
+- Fixed a false positive in :doc:`cppcoreguidelines-pro-type-member-init
+  ` when any member
+  of the anonymous union has been initialized.
+
 - Fixed a false positive in :doc:`cppcoreguidelines-virtual-class-destructor
   ` involving
   ``final`` classes. The check will not diagnose classes marked ``final``, since
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -61,6 +61,17 @@
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+SmallPtrSetImpl ) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+// Erase all members in a union if any member of it is initialized.
+for (const auto *F : R->fields())
+  FieldDecls.erase(F);
+  } else
+FieldDecls.erase(M);
+}
+
 void removeFieldsInitializedInBody(
 const Stmt , ASTContext ,
 SmallPtrSetImpl ) {
@@ -70,7 +81,7 @@
 hasLHS(memberExpr(member(fieldDecl().bind("fieldDecl")),
 Stmt, Context);
   for (const auto  : Matches)
-FieldDecls.erase(Match.getNodeAs("fieldDecl"));
+removeFieldInitialized(Match.getNodeAs("fieldDecl"), FieldDecls);
 }
 
 StringRef getName(const FieldDecl *Field) { return Field->getName(); }
@@ -418,13 +429,18 @@
 
   // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet FieldsToInit;
-  forEachField(ClassDecl, ClassDecl.fields(), [&](const FieldDecl *F) {
+  bool AnyMemberHasInitPerUnion = false;
+  forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (IgnoreArrays && F->getType()->isArrayType())
   return;
+if (F->hasInClassInitializer() && F->getParent()->isUnion())
+  AnyMemberHasInitPerUnion = true;
 if (!F->hasInClassInitializer() &&
 utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
 Context) &&
-!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield())
+!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield() &&
+!AnyMemberHasInitPerUnion)
   FieldsToInit.insert(F);
   });
   if (FieldsToInit.empty())
@@ -437,7 +453,7 @@
   if (Init->isAnyMemberInitializer() && Init->isWritten()) {
 if (IsUnion)
   return; // We can only initialize one member of a union.
-FieldsToInit.erase(Init->getAnyMember());
+removeFieldInitialized(Init->getAnyMember(), FieldsToInit);
   }
 }
 removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
@@ -478,7 +494,7 @@
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet FieldsToFix;
-  bool AnyMemberHasInitPerUnion = false;
+  AnyMemberHasInitPerUnion = false;
   forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
  AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 440517.
Sockke added a comment.

rebased.


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

https://reviews.llvm.org/D127293

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
@@ -552,3 +552,19 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  union {
+int a;
+int b;
+  };
+};
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -61,6 +61,17 @@
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+SmallPtrSetImpl ) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+// Erase all members in a union if any member of it is initialized.
+for (const auto *F : R->fields())
+  FieldDecls.erase(F);
+  } else
+FieldDecls.erase(M);
+}
+
 void removeFieldsInitializedInBody(
 const Stmt , ASTContext ,
 SmallPtrSetImpl ) {
@@ -70,7 +81,7 @@
 hasLHS(memberExpr(member(fieldDecl().bind("fieldDecl")),
 Stmt, Context);
   for (const auto  : Matches)
-FieldDecls.erase(Match.getNodeAs("fieldDecl"));
+removeFieldInitialized(Match.getNodeAs("fieldDecl"), 
FieldDecls);
 }
 
 StringRef getName(const FieldDecl *Field) { return Field->getName(); }
@@ -418,13 +429,18 @@
 
   // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet FieldsToInit;
-  forEachField(ClassDecl, ClassDecl.fields(), [&](const FieldDecl *F) {
+  bool AnyMemberHasInitPerUnion = false;
+  forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (IgnoreArrays && F->getType()->isArrayType())
   return;
+if (F->hasInClassInitializer() && F->getParent()->isUnion())
+  AnyMemberHasInitPerUnion = true;
 if (!F->hasInClassInitializer() &&
 utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
 Context) &&
-!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield())
+!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield() &&
+!AnyMemberHasInitPerUnion)
   FieldsToInit.insert(F);
   });
   if (FieldsToInit.empty())
@@ -437,7 +453,7 @@
   if (Init->isAnyMemberInitializer() && Init->isWritten()) {
 if (IsUnion)
   return; // We can only initialize one member of a union.
-FieldsToInit.erase(Init->getAnyMember());
+removeFieldInitialized(Init->getAnyMember(), FieldsToInit);
   }
 }
 removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
@@ -478,7 +494,7 @@
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet FieldsToFix;
-  bool AnyMemberHasInitPerUnion = false;
+  AnyMemberHasInitPerUnion = false;
   forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
  AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-member-init.cpp
@@ -552,3 +552,19 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  union {
+int a;
+int b;
+  };
+};
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -61,6 +61,17 @@
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+SmallPtrSetImpl ) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+// Erase all members in a union if any member of it is initialized.

[PATCH] D128697: [clang-tidy] Add new check `bugprone-unhandled-exception-at-sto`

[gehry via Phabricator via cfe-commits]

Sockke created this revision.
Sockke added a reviewer: aaron.ballman.
Herald added subscribers: carlosgalvezp, xazax.hun, mgorny.
Herald added a project: All.
Sockke requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Calls to **stoi** and **stod** families of functions may throw exceptions so 
they need to be surrounded by try/catch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128697

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledExceptionAtStoCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledExceptionAtStoCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone/unhandled-exception-at-sto.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-exception-at-sto.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-exception-at-sto.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-exception-at-sto.cpp
@@ -0,0 +1,128 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-exception-at-sto %t
+
+namespace std {
+
+template 
+struct basic_string {
+  basic_string(const Char *);
+  ~basic_string();
+};
+typedef basic_string string;
+
+typedef unsigned int size_t;
+
+int stoi(const std::string , size_t *pos = nullptr, int base = 10) { return 0; }
+long stol(const std::string , size_t *pos = nullptr, int base = 10) { return 0; }
+long long stoll(const std::string , size_t *pos = nullptr, int base = 10) { return 0; }
+
+float stof(const std::string , size_t *pos = nullptr) { return 0; }
+double stod(const std::string , size_t *pos = nullptr) { return 0; }
+long double stold(const std::string , size_t *pos = nullptr) { return 0; }
+
+struct exception {};
+struct logic_error : public exception {};
+struct invalid_argument : public logic_error {};
+struct out_of_range : public logic_error {};
+
+} // namespace std
+
+void test() {
+  std::string s("123");
+
+  int a = std::stoi(s);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: missing exception handler for invalid argument and out of range at 'std::stoi' [bugprone-unhandled-exception-at-sto]
+  long b = std::stol(s);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: missing exception handler for invalid argument and out of range at 'std::stol' [bugprone-unhandled-exception-at-sto]
+  long long c = std::stoll(s);
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: missing exception handler for invalid argument and out of range at 'std::stoll' [bugprone-unhandled-exception-at-sto]
+  float d = std::stof(s);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: missing exception handler for invalid argument and out of range at 'std::stof' [bugprone-unhandled-exception-at-sto]
+  double e = std::stod(s);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: missing exception handler for invalid argument and out of range at 'std::stod' [bugprone-unhandled-exception-at-sto]
+  long double f = std::stold(s);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: missing exception handler for invalid argument and out of range at 'std::stold' [bugprone-unhandled-exception-at-sto]
+
+  try {
+int v = std::stoi(s);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: missing exception handler for invalid argument at 'std::stoi' [bugprone-unhandled-exception-at-sto]
+  } catch (std::out_of_range) {
+  }
+  try {
+int v = std::stoi(s);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: missing exception handler for invalid argument at 'std::stoi' [bugprone-unhandled-exception-at-sto]
+  } catch (std::out_of_range &) {
+  }
+  try {
+int v = std::stoi(s);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: missing exception handler for invalid argument at 'std::stoi' [bugprone-unhandled-exception-at-sto]
+  } catch (const std::out_of_range &) {
+  }
+  try {
+int v = std::stoi(s);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: missing exception handler for invalid argument and out of range at 'std::stoi' [bugprone-unhandled-exception-at-sto]
+  } catch (std::out_of_range *) {
+  }
+
+  try {
+int v = std::stoi(s);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: missing exception handler for out of range at 'std::stoi' [bugprone-unhandled-exception-at-sto]
+  } catch (std::invalid_argument) {
+  }
+  try {
+int v = std::stoi(s);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: missing exception handler for out of range at 'std::stoi' [bugprone-unhandled-exception-at-sto]
+  } catch (std::invalid_argument &) {
+  }
+  try {
+int v = std::stoi(s);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: missing exception handler for out of range at 'std::stoi' [bugprone-unhandled-exception-at-sto]
+  } catch (const std::invalid_argument 

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 438295.
Sockke added a comment.

rebased.


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

https://reviews.llvm.org/D127293

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
@@ -552,3 +552,19 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  union {
+int a;
+int b;
+  };
+};
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -61,6 +61,17 @@
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+SmallPtrSetImpl ) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+// Erase all members in a union if any member of it is initialized.
+for (const auto *F : R->fields())
+  FieldDecls.erase(F);
+  } else
+FieldDecls.erase(M);
+}
+
 void removeFieldsInitializedInBody(
 const Stmt , ASTContext ,
 SmallPtrSetImpl ) {
@@ -70,7 +81,7 @@
 hasLHS(memberExpr(member(fieldDecl().bind("fieldDecl")),
 Stmt, Context);
   for (const auto  : Matches)
-FieldDecls.erase(Match.getNodeAs("fieldDecl"));
+removeFieldInitialized(Match.getNodeAs("fieldDecl"), 
FieldDecls);
 }
 
 StringRef getName(const FieldDecl *Field) { return Field->getName(); }
@@ -418,13 +429,18 @@
 
   // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet FieldsToInit;
-  forEachField(ClassDecl, ClassDecl.fields(), [&](const FieldDecl *F) {
+  bool AnyMemberHasInitPerUnion = false;
+  forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (IgnoreArrays && F->getType()->isArrayType())
   return;
+if (F->hasInClassInitializer() && F->getParent()->isUnion())
+  AnyMemberHasInitPerUnion = true;
 if (!F->hasInClassInitializer() &&
 utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
 Context) &&
-!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield())
+!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield() &&
+!AnyMemberHasInitPerUnion)
   FieldsToInit.insert(F);
   });
   if (FieldsToInit.empty())
@@ -437,7 +453,7 @@
   if (Init->isAnyMemberInitializer() && Init->isWritten()) {
 if (IsUnion)
   return; // We can only initialize one member of a union.
-FieldsToInit.erase(Init->getAnyMember());
+removeFieldInitialized(Init->getAnyMember(), FieldsToInit);
   }
 }
 removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
@@ -478,7 +494,7 @@
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet FieldsToFix;
-  bool AnyMemberHasInitPerUnion = false;
+  AnyMemberHasInitPerUnion = false;
   forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
  AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
@@ -552,3 +552,19 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  union {
+int a;
+int b;
+  };
+};
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -61,6 +61,17 @@
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+SmallPtrSetImpl ) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+// Erase all members in a union if any member of it is initialized.

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 435409.
Sockke edited the summary of this revision.
Sockke added a comment.

Fix test file.


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

https://reviews.llvm.org/D127293

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
@@ -552,3 +552,19 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  union {
+int a;
+int b;
+  };
+};
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -61,6 +61,17 @@
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+SmallPtrSetImpl ) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+// Erase all members in a union if any member of it is initialized.
+for (const auto *F : R->fields())
+  FieldDecls.erase(F);
+  } else
+FieldDecls.erase(M);
+}
+
 void removeFieldsInitializedInBody(
 const Stmt , ASTContext ,
 SmallPtrSetImpl ) {
@@ -70,7 +81,7 @@
 hasLHS(memberExpr(member(fieldDecl().bind("fieldDecl")),
 Stmt, Context);
   for (const auto  : Matches)
-FieldDecls.erase(Match.getNodeAs("fieldDecl"));
+removeFieldInitialized(Match.getNodeAs("fieldDecl"), 
FieldDecls);
 }
 
 StringRef getName(const FieldDecl *Field) { return Field->getName(); }
@@ -418,13 +429,18 @@
 
   // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet FieldsToInit;
-  forEachField(ClassDecl, ClassDecl.fields(), [&](const FieldDecl *F) {
+  bool AnyMemberHasInitPerUnion = false;
+  forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (IgnoreArrays && F->getType()->isArrayType())
   return;
+if (F->hasInClassInitializer() && F->getParent()->isUnion())
+  AnyMemberHasInitPerUnion = true;
 if (!F->hasInClassInitializer() &&
 utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
 Context) &&
-!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield())
+!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield() &&
+!AnyMemberHasInitPerUnion)
   FieldsToInit.insert(F);
   });
   if (FieldsToInit.empty())
@@ -437,7 +453,7 @@
   if (Init->isAnyMemberInitializer() && Init->isWritten()) {
 if (IsUnion)
   return; // We can only initialize one member of a union.
-FieldsToInit.erase(Init->getAnyMember());
+removeFieldInitialized(Init->getAnyMember(), FieldsToInit);
   }
 }
 removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
@@ -478,7 +494,7 @@
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet FieldsToFix;
-  bool AnyMemberHasInitPerUnion = false;
+  AnyMemberHasInitPerUnion = false;
   forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
  AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
@@ -552,3 +552,19 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  union {
+int a;
+int b;
+  };
+};
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -61,6 +61,17 @@
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+SmallPtrSetImpl ) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+// Erase all 

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke created this revision.
Sockke added reviewers: aaron.ballman, MTC.
Herald added subscribers: carlosgalvezp, shchenz, kbarton, xazax.hun, nemanjai.
Herald added a project: All.
Sockke requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

If a union member is initialized, other members are still recorded in the 
container to be initialized.  This patch fixes this behavior.
Reference issue: https://github.com/llvm/llvm-project/issues/54748


https://reviews.llvm.org/D127293

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
@@ -552,3 +552,21 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  // CHECK-MESSAGES-NOT: warning:
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  // CHECK-MESSAGES-NOT: warning:
+  union {
+int a;
+int b;
+  };
+};
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -61,6 +61,17 @@
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+SmallPtrSetImpl ) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+// Erase all members in a union if any member of it is initialized.
+for (const auto *F : R->fields())
+  FieldDecls.erase(F);
+  } else
+FieldDecls.erase(M);
+}
+
 void removeFieldsInitializedInBody(
 const Stmt , ASTContext ,
 SmallPtrSetImpl ) {
@@ -70,7 +81,7 @@
 hasLHS(memberExpr(member(fieldDecl().bind("fieldDecl")),
 Stmt, Context);
   for (const auto  : Matches)
-FieldDecls.erase(Match.getNodeAs("fieldDecl"));
+removeFieldInitialized(Match.getNodeAs("fieldDecl"), 
FieldDecls);
 }
 
 StringRef getName(const FieldDecl *Field) { return Field->getName(); }
@@ -418,13 +429,18 @@
 
   // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet FieldsToInit;
-  forEachField(ClassDecl, ClassDecl.fields(), [&](const FieldDecl *F) {
+  bool AnyMemberHasInitPerUnion = false;
+  forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (IgnoreArrays && F->getType()->isArrayType())
   return;
+if (F->hasInClassInitializer() && F->getParent()->isUnion())
+  AnyMemberHasInitPerUnion = true;
 if (!F->hasInClassInitializer() &&
 utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
 Context) &&
-!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield())
+!isEmpty(Context, F->getType()) && !F->isUnnamedBitfield() &&
+!AnyMemberHasInitPerUnion)
   FieldsToInit.insert(F);
   });
   if (FieldsToInit.empty())
@@ -437,7 +453,7 @@
   if (Init->isAnyMemberInitializer() && Init->isWritten()) {
 if (IsUnion)
   return; // We can only initialize one member of a union.
-FieldsToInit.erase(Init->getAnyMember());
+removeFieldInitialized(Init->getAnyMember(), FieldsToInit);
   }
 }
 removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
@@ -478,7 +494,7 @@
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet FieldsToFix;
-  bool AnyMemberHasInitPerUnion = false;
+  AnyMemberHasInitPerUnion = false;
   forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
  AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
@@ -552,3 +552,21 @@
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  // CHECK-MESSAGES-NOT: warning:
+  union {
+int a = 0;
+int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  // CHECK-MESSAGES-NOT: warning:
+  union {
+int a;
+int b;
+  };
+};
Index: 

[PATCH] D123924: [clang-apply-replacements] Added an option to ignore insert conflict.

[gehry via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3f3a235aa2e6: [clang-apply-replacements] Added an option to 
ignore insert conflict. (authored by Sockke).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123924

Files:
  
clang-tools-extra/clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
  
clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
  clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
  clang/include/clang/Tooling/Refactoring/AtomicChange.h

Index: clang/include/clang/Tooling/Refactoring/AtomicChange.h
===
--- clang/include/clang/Tooling/Refactoring/AtomicChange.h
+++ clang/include/clang/Tooling/Refactoring/AtomicChange.h
@@ -116,6 +116,8 @@
   /// Returns a const reference to existing replacements.
   const Replacements () const { return Replaces; }
 
+  Replacements () { return Replaces; }
+
   llvm::ArrayRef getInsertedHeaders() const {
 return InsertedHeaders;
   }
Index: clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
@@ -0,0 +1,5 @@
+// RUN: mkdir -p %T/Inputs/ignore-conflict
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/ignore-conflict/ignore-conflict.cpp > %T/Inputs/ignore-conflict/ignore-conflict.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/ignore-conflict#" %S/Inputs/ignore-conflict/file1.yaml > %T/Inputs/ignore-conflict/file1.yaml
+// RUN: clang-apply-replacements --ignore-insert-conflict %T/Inputs/ignore-conflict
+// RUN: FileCheck -input-file=%T/Inputs/ignore-conflict/ignore-conflict.cpp %S/Inputs/ignore-conflict/ignore-conflict.cpp
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
@@ -0,0 +1,4 @@
+class MyType {};
+// CHECK: #include 
+// CHECK-NEXT: #include 
+// CEHCK-NEXT: class MyType {};
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
@@ -0,0 +1,24 @@
+---
+MainSourceFile: ignore-conflict.cpp
+Diagnostics:
+  - DiagnosticName: test-ignore-conflict-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/ignore-conflict.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/ignore-conflict.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+  - DiagnosticName: test-ignore-conflict-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/ignore-conflict.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/ignore-conflict.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+...
Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -180,6 +180,7 @@
 def apply_fixes(args, clang_apply_replacements_binary, tmpdir):
   """Calls clang-apply-fixes on a given directory."""
   invocation = [clang_apply_replacements_binary]
+  invocation.append('-ignore-insert-conflict')
   if args.format:
 invocation.append('-format')
   if args.style:
Index: clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
===
--- clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -42,6 +42,11 @@
  "merging/replacing."),
 cl::init(false), cl::cat(ReplacementCategory));
 
+static cl::opt IgnoreInsertConflict(
+"ignore-insert-conflict",
+cl::desc("Ignore insert conflict and keep running to fix."),
+cl::init(false), cl::cat(ReplacementCategory));
+
 static cl::opt DoFormat(
 "format",
 cl::desc("Enable formatting of code changed by applying replacements.\n"
@@ -131,7 +136,7 @@
   SourceManager SM(Diagnostics, Files);
 
   

[PATCH] D123924: [clang-apply-replacements] Added an option to ignore insert conflict.

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 432829.

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

https://reviews.llvm.org/D123924

Files:
  
clang-tools-extra/clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
  
clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
  clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
  clang/include/clang/Tooling/Refactoring/AtomicChange.h

Index: clang/include/clang/Tooling/Refactoring/AtomicChange.h
===
--- clang/include/clang/Tooling/Refactoring/AtomicChange.h
+++ clang/include/clang/Tooling/Refactoring/AtomicChange.h
@@ -116,6 +116,8 @@
   /// Returns a const reference to existing replacements.
   const Replacements () const { return Replaces; }
 
+  Replacements () { return Replaces; }
+
   llvm::ArrayRef getInsertedHeaders() const {
 return InsertedHeaders;
   }
Index: clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
@@ -0,0 +1,5 @@
+// RUN: mkdir -p %T/Inputs/ignore-conflict
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/ignore-conflict/ignore-conflict.cpp > %T/Inputs/ignore-conflict/ignore-conflict.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/ignore-conflict#" %S/Inputs/ignore-conflict/file1.yaml > %T/Inputs/ignore-conflict/file1.yaml
+// RUN: clang-apply-replacements --ignore-insert-conflict %T/Inputs/ignore-conflict
+// RUN: FileCheck -input-file=%T/Inputs/ignore-conflict/ignore-conflict.cpp %S/Inputs/ignore-conflict/ignore-conflict.cpp
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
@@ -0,0 +1,4 @@
+class MyType {};
+// CHECK: #include 
+// CHECK-NEXT: #include 
+// CEHCK-NEXT: class MyType {};
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
@@ -0,0 +1,24 @@
+---
+MainSourceFile: ignore-conflict.cpp
+Diagnostics:
+  - DiagnosticName: test-ignore-conflict-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/ignore-conflict.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/ignore-conflict.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+  - DiagnosticName: test-ignore-conflict-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/ignore-conflict.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/ignore-conflict.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+...
Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -180,6 +180,7 @@
 def apply_fixes(args, clang_apply_replacements_binary, tmpdir):
   """Calls clang-apply-fixes on a given directory."""
   invocation = [clang_apply_replacements_binary]
+  invocation.append('-ignore-insert-conflict')
   if args.format:
 invocation.append('-format')
   if args.style:
Index: clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
===
--- clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -42,6 +42,11 @@
  "merging/replacing."),
 cl::init(false), cl::cat(ReplacementCategory));
 
+static cl::opt IgnoreInsertConflict(
+"ignore-insert-conflict",
+cl::desc("Ignore insert conflict and keep running to fix."),
+cl::init(false), cl::cat(ReplacementCategory));
+
 static cl::opt DoFormat(
 "format",
 cl::desc("Enable formatting of code changed by applying replacements.\n"
@@ -131,7 +136,7 @@
   SourceManager SM(Diagnostics, Files);
 
   FileToChangesMap Changes;
-  if (!mergeAndDeduplicate(TURs, TUDs, Changes, SM))
+  if (!mergeAndDeduplicate(TURs, TUDs, Changes, SM, IgnoreInsertConflict))
 return 1;
 
   

[PATCH] D116593: Fix `performance-unnecessary-value-param` for template specialization

[gehry via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc98b3a8cd985: Fix `performance-unnecessary-value-param` for 
template specialization (authored by Sockke).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116593

Files:
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t
+// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t -- -- -fno-delayed-template-parsing
 
 // CHECK-FIXES: #include 
 
@@ -109,7 +109,7 @@
 
 template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-FIXES-NOT: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
@@ -381,3 +381,24 @@
   // CHECK-FIXES: void templateFunction(ExpensiveToCopyType E) {
   E.constReference();
 }
+
+template 
+T templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: T templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return T();
+}
+
+template <>
+bool templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:57: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: bool templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return true;
+}
+
+template <>
+int templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:56: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: int templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return 0;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
@@ -69,7 +69,7 @@
 
 template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-FIXES-NOT: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -202,6 +202,10 @@
   ` to simplify expressions
   using DeMorgan's Theorem.
 
+- Fixed a crash in :doc:`performance-unnecessary-value-param
+  ` when the specialization
+  template has an unnecessary value paramter. Removed the fix for a template.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -51,18 +51,6 @@
   return Matches.empty();
 }
 
-bool isExplicitTemplateSpecialization(const FunctionDecl ) {
-  if (const auto *SpecializationInfo = Function.getTemplateSpecializationInfo())
-if (SpecializationInfo->getTemplateSpecializationKind() ==
-TSK_ExplicitSpecialization)
-  return true;
-  if (const auto *Method = llvm::dyn_cast())
-if (Method->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization &&
-Method->getMemberSpecializationInfo()->isExplicitSpecialization())
-  return true;
-  return false;
-}
-
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -147,11 +135,12 @@
   // 2. the function is virtual as it might break overrides
   // 3. the function is referenced outside of a call expression within the
   //compilation unit as the signature change could introduce build errors.
-  // 4. the function is 

[PATCH] D123924: [clang-apply-replacements] Added an option to ignore insert conflict.

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 432522.
Sockke added a comment.

rebased.


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

https://reviews.llvm.org/D123924

Files:
  
clang-tools-extra/clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
  
clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
  clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
  clang/include/clang/Tooling/Refactoring/AtomicChange.h

Index: clang/include/clang/Tooling/Refactoring/AtomicChange.h
===
--- clang/include/clang/Tooling/Refactoring/AtomicChange.h
+++ clang/include/clang/Tooling/Refactoring/AtomicChange.h
@@ -116,6 +116,8 @@
   /// Returns a const reference to existing replacements.
   const Replacements () const { return Replaces; }
 
+  Replacements () { return Replaces; }
+
   llvm::ArrayRef getInsertedHeaders() const {
 return InsertedHeaders;
   }
Index: clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
@@ -0,0 +1,5 @@
+// RUN: mkdir -p %T/Inputs/ignore-conflict
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/ignore-conflict/ignore-conflict.cpp > %T/Inputs/ignore-conflict/ignore-conflict.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/ignore-conflict#" %S/Inputs/ignore-conflict/file1.yaml > %T/Inputs/ignore-conflict/file1.yaml
+// RUN: clang-apply-replacements --ignore-insert-conflict %T/Inputs/ignore-conflict
+// RUN: FileCheck -input-file=%T/Inputs/ignore-conflict/ignore-conflict.cpp %S/Inputs/ignore-conflict/ignore-conflict.cpp
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
@@ -0,0 +1,4 @@
+class MyType {};
+// CHECK: #include 
+// CHECK-NEXT: #include 
+// CEHCK-NEXT: class MyType {};
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
@@ -0,0 +1,24 @@
+---
+MainSourceFile: ignore-conflict.cpp
+Diagnostics:
+  - DiagnosticName: test-ignore-conflict-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/ignore-conflict.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/ignore-conflict.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+  - DiagnosticName: test-ignore-conflict-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/ignore-conflict.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/ignore-conflict.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+...
Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -180,6 +180,7 @@
 def apply_fixes(args, clang_apply_replacements_binary, tmpdir):
   """Calls clang-apply-fixes on a given directory."""
   invocation = [clang_apply_replacements_binary]
+  invocation.append('-ignore-insert-conflict')
   if args.format:
 invocation.append('-format')
   if args.style:
Index: clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
===
--- clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -42,6 +42,11 @@
  "merging/replacing."),
 cl::init(false), cl::cat(ReplacementCategory));
 
+static cl::opt IgnoreInsertConflict(
+"ignore-insert-conflict",
+cl::desc("Ignore insert conflict and keep running to fix."),
+cl::init(false), cl::cat(ReplacementCategory));
+
 static cl::opt DoFormat(
 "format",
 cl::desc("Enable formatting of code changed by applying replacements.\n"
@@ -131,7 +136,7 @@
   SourceManager SM(Diagnostics, Files);
 
   FileToChangesMap Changes;
-  if (!mergeAndDeduplicate(TURs, TUDs, Changes, SM))
+  if (!mergeAndDeduplicate(TURs, TUDs, Changes, SM, IgnoreInsertConflict))

[PATCH] D116593: Fix `performance-unnecessary-value-param` for template specialization

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 432520.
Sockke added a comment.

Rebased and added '-fno-delayed-template-parsing' option for the test file.


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

https://reviews.llvm.org/D116593

Files:
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t
+// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t -- -- -fno-delayed-template-parsing
 
 // CHECK-FIXES: #include 
 
@@ -109,7 +109,7 @@
 
 template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-FIXES-NOT: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
@@ -381,3 +381,24 @@
   // CHECK-FIXES: void templateFunction(ExpensiveToCopyType E) {
   E.constReference();
 }
+
+template 
+T templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: T templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return T();
+}
+
+template <>
+bool templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:57: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: bool templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return true;
+}
+
+template <>
+int templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:56: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: int templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return 0;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
@@ -69,7 +69,7 @@
 
 template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-FIXES-NOT: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -202,6 +202,10 @@
   ` to simplify expressions
   using DeMorgan's Theorem.
 
+- Fixed a crash in :doc:`performance-unnecessary-value-param
+  ` when the specialization
+  template has an unnecessary value paramter. Removed the fix for a template.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -51,18 +51,6 @@
   return Matches.empty();
 }
 
-bool isExplicitTemplateSpecialization(const FunctionDecl ) {
-  if (const auto *SpecializationInfo = Function.getTemplateSpecializationInfo())
-if (SpecializationInfo->getTemplateSpecializationKind() ==
-TSK_ExplicitSpecialization)
-  return true;
-  if (const auto *Method = llvm::dyn_cast())
-if (Method->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization &&
-Method->getMemberSpecializationInfo()->isExplicitSpecialization())
-  return true;
-  return false;
-}
-
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -147,11 +135,12 @@
   // 2. the function is virtual as it might break overrides
   // 3. the function is referenced outside of a call expression within the
   //compilation unit as the signature change could introduce build errors.
-  // 4. the function is an explicit template specialization.
+  // 4. the function is a primary template or an 

[PATCH] D116593: Fix `performance-unnecessary-value-param` for template specialization

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

I'm not quite sure why the test case is not passing on Windows, could you 
please check this problem? @flx


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

https://reviews.llvm.org/D116593

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

[PATCH] D123924: [clang-apply-replacements] Added an option to ignore insert conflict.

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Hi, Could anyone please review this diff?


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

https://reviews.llvm.org/D123924

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

[PATCH] D116593: Fix `performance-unnecessary-value-param` for template specialization

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 430621.
Sockke added a comment.

rebased.


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

https://reviews.llvm.org/D116593

Files:
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
@@ -109,7 +109,7 @@
 
 template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-FIXES-NOT: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
@@ -381,3 +381,24 @@
   // CHECK-FIXES: void templateFunction(ExpensiveToCopyType E) {
   E.constReference();
 }
+
+template 
+T templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: T templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return T();
+}
+
+template <>
+bool templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:57: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: bool templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return true;
+}
+
+template <>
+int templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:56: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: int templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return 0;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
@@ -69,7 +69,7 @@
 
 template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-FIXES-NOT: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -185,6 +185,10 @@
   ` when the parameter is
   referenced by an lvalue.
 
+- Fixed a crash in :doc:`performance-unnecessary-value-param
+  ` when the specialization
+  template has an unnecessary value paramter. Removed the fix for a template.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -51,18 +51,6 @@
   return Matches.empty();
 }
 
-bool isExplicitTemplateSpecialization(const FunctionDecl ) {
-  if (const auto *SpecializationInfo = Function.getTemplateSpecializationInfo())
-if (SpecializationInfo->getTemplateSpecializationKind() ==
-TSK_ExplicitSpecialization)
-  return true;
-  if (const auto *Method = llvm::dyn_cast())
-if (Method->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization &&
-Method->getMemberSpecializationInfo()->isExplicitSpecialization())
-  return true;
-  return false;
-}
-
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -147,11 +135,12 @@
   // 2. the function is virtual as it might break overrides
   // 3. the function is referenced outside of a call expression within the
   //compilation unit as the signature change could introduce build errors.
-  // 4. the function is an explicit template specialization.
+  // 4. the function is a primary template or an explicit template
+  // specialization.
   const auto *Method = llvm::dyn_cast(Function);
   if (Param->getBeginLoc().isMacroID() || (Method && Method->isVirtual()) ||
   isReferencedOutsideOfCallExpr(*Function, *Result.Context) ||
-  isExplicitTemplateSpecialization(*Function))
+  

[PATCH] D123924: [clang-apply-replacements] Added an option to ignore insert conflict.

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping.


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

https://reviews.llvm.org/D123924

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

[PATCH] D123924: [clang-apply-replacements] Added an option to ignore insert conflict.

[gehry via Phabricator via cfe-commits]

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

Friendly ping.


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

https://reviews.llvm.org/D123924

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

[PATCH] D123924: [clang-apply-replacements] Added an option to ignore insert conflict.

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 425166.
Sockke added a comment.

Rename the function.


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

https://reviews.llvm.org/D123924

Files:
  
clang-tools-extra/clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
  
clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
  clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
  clang/include/clang/Tooling/Refactoring/AtomicChange.h

Index: clang/include/clang/Tooling/Refactoring/AtomicChange.h
===
--- clang/include/clang/Tooling/Refactoring/AtomicChange.h
+++ clang/include/clang/Tooling/Refactoring/AtomicChange.h
@@ -116,6 +116,8 @@
   /// Returns a const reference to existing replacements.
   const Replacements () const { return Replaces; }
 
+  Replacements () { return Replaces; }
+
   llvm::ArrayRef getInsertedHeaders() const {
 return InsertedHeaders;
   }
Index: clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
@@ -0,0 +1,5 @@
+// RUN: mkdir -p %T/Inputs/ignore-conflict
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/ignore-conflict/ignore-conflict.cpp > %T/Inputs/ignore-conflict/ignore-conflict.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/ignore-conflict#" %S/Inputs/ignore-conflict/file1.yaml > %T/Inputs/ignore-conflict/file1.yaml
+// RUN: clang-apply-replacements --ignore-insert-conflict %T/Inputs/ignore-conflict
+// RUN: FileCheck -input-file=%T/Inputs/ignore-conflict/ignore-conflict.cpp %S/Inputs/ignore-conflict/ignore-conflict.cpp
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
@@ -0,0 +1,4 @@
+class MyType {};
+// CHECK: #include 
+// CHECK-NEXT: #include 
+// CEHCK-NEXT: class MyType {};
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
@@ -0,0 +1,24 @@
+---
+MainSourceFile: ignore-conflict.cpp
+Diagnostics:
+  - DiagnosticName: test-ignore-conflict-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/ignore-conflict.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/ignore-conflict.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+  - DiagnosticName: test-ignore-conflict-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/ignore-conflict.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/ignore-conflict.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+...
Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -178,6 +178,7 @@
 def apply_fixes(args, clang_apply_replacements_binary, tmpdir):
   """Calls clang-apply-fixes on a given directory."""
   invocation = [clang_apply_replacements_binary]
+  invocation.append('-ignore-insert-conflict')
   if args.format:
 invocation.append('-format')
   if args.style:
Index: clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
===
--- clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -42,6 +42,11 @@
  "merging/replacing."),
 cl::init(false), cl::cat(ReplacementCategory));
 
+static cl::opt IgnoreInsertConflict(
+"ignore-insert-conflict",
+cl::desc("Ignore insert conflict and keep running to fix."),
+cl::init(false), cl::cat(ReplacementCategory));
+
 static cl::opt DoFormat(
 "format",
 cl::desc("Enable formatting of code changed by applying replacements.\n"
@@ -131,7 +136,7 @@
   SourceManager SM(Diagnostics, Files);
 
   FileToChangesMap Changes;
-  if (!mergeAndDeduplicate(TURs, TUDs, Changes, SM))
+  if (!mergeAndDeduplicate(TURs, TUDs, Changes, SM, 

[PATCH] D123924: [clang-apply-replacements] Added an option to ignore insert conflict.

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 423359.
Sockke retitled this revision from "[clang-apply-replacements] Added support 
for adjusting conflict offset." to "[clang-apply-replacements] Added an option 
to ignore insert conflict.".

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

https://reviews.llvm.org/D123924

Files:
  
clang-tools-extra/clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
  
clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
  clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
  clang/include/clang/Tooling/Refactoring/AtomicChange.h

Index: clang/include/clang/Tooling/Refactoring/AtomicChange.h
===
--- clang/include/clang/Tooling/Refactoring/AtomicChange.h
+++ clang/include/clang/Tooling/Refactoring/AtomicChange.h
@@ -116,6 +116,8 @@
   /// Returns a const reference to existing replacements.
   const Replacements () const { return Replaces; }
 
+  Replacements () { return Replaces; }
+
   llvm::ArrayRef getInsertedHeaders() const {
 return InsertedHeaders;
   }
Index: clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
@@ -0,0 +1,5 @@
+// RUN: mkdir -p %T/Inputs/ignore-conflict
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/ignore-conflict/ignore-conflict.cpp > %T/Inputs/ignore-conflict/ignore-conflict.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/ignore-conflict#" %S/Inputs/ignore-conflict/file1.yaml > %T/Inputs/ignore-conflict/file1.yaml
+// RUN: clang-apply-replacements --ignore-insert-conflict %T/Inputs/ignore-conflict
+// RUN: FileCheck -input-file=%T/Inputs/ignore-conflict/ignore-conflict.cpp %S/Inputs/ignore-conflict/ignore-conflict.cpp
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
@@ -0,0 +1,4 @@
+class MyType {};
+// CHECK: #include 
+// CHECK-NEXT: #include 
+// CEHCK-NEXT: class MyType {};
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
@@ -0,0 +1,24 @@
+---
+MainSourceFile: ignore-conflict.cpp
+Diagnostics:
+  - DiagnosticName: test-ignore-conflict-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/ignore-conflict.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/ignore-conflict.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+  - DiagnosticName: test-ignore-conflict-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/ignore-conflict.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/ignore-conflict.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+...
Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -177,6 +177,7 @@
 def apply_fixes(args, clang_apply_replacements_binary, tmpdir):
   """Calls clang-apply-fixes on a given directory."""
   invocation = [clang_apply_replacements_binary]
+  invocation.append('-ignore-insert-conflict')
   if args.format:
 invocation.append('-format')
   if args.style:
Index: clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
===
--- clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -42,6 +42,11 @@
  "merging/replacing."),
 cl::init(false), cl::cat(ReplacementCategory));
 
+static cl::opt IgnoreInsertConflict(
+"ignore-insert-conflict",
+cl::desc("Ignore insert conflict and keep running to fix."),
+cl::init(false), cl::cat(ReplacementCategory));
+
 static cl::opt DoFormat(
 "format",
 cl::desc("Enable formatting of code changed by applying replacements.\n"
@@ -131,7 +136,7 @@
   SourceManager SM(Diagnostics, Files);
 
   

[PATCH] D123924: [clang-apply-replacements] Added support for adjusting conflict offset.

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

It stands to reason that there should be no insert conflicts in the YAML 
generated by clang-tidy except for the header insertion. The two checks that 
caused the conflict should be fixed in clang-tidy.


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

https://reviews.llvm.org/D123924

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

[PATCH] D123924: [clang-apply-replacements] Added support for adjusting conflict offset.

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 423355.
Sockke retitled this revision from "[WIP][clang-apply-replacements] Added 
support for adjusting conflict offset." to "[clang-apply-replacements] Added 
support for adjusting conflict offset.".
Sockke added a comment.
Herald added a subscriber: carlosgalvezp.

Add an option to ignore insert conflict.


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

https://reviews.llvm.org/D123924

Files:
  
clang-tools-extra/clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
  
clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
  clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
  clang/include/clang/Tooling/Refactoring/AtomicChange.h

Index: clang/include/clang/Tooling/Refactoring/AtomicChange.h
===
--- clang/include/clang/Tooling/Refactoring/AtomicChange.h
+++ clang/include/clang/Tooling/Refactoring/AtomicChange.h
@@ -116,6 +116,8 @@
   /// Returns a const reference to existing replacements.
   const Replacements () const { return Replaces; }
 
+  Replacements () { return Replaces; }
+
   llvm::ArrayRef getInsertedHeaders() const {
 return InsertedHeaders;
   }
Index: clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp
@@ -0,0 +1,5 @@
+// RUN: mkdir -p %T/Inputs/ignore-conflict
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/ignore-conflict/ignore-conflict.cpp > %T/Inputs/ignore-conflict/ignore-conflict.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/ignore-conflict#" %S/Inputs/ignore-conflict/file1.yaml > %T/Inputs/ignore-conflict/file1.yaml
+// RUN: clang-apply-replacements --ignore-insert-conflict %T/Inputs/ignore-conflict
+// RUN: FileCheck -input-file=%T/Inputs/ignore-conflict/ignore-conflict.cpp %S/Inputs/ignore-conflict/ignore-conflict.cpp
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp
@@ -0,0 +1,4 @@
+class MyType {};
+// CHECK: #include 
+// CHECK-NEXT: #include 
+// CEHCK-NEXT: class MyType {};
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml
@@ -0,0 +1,24 @@
+---
+MainSourceFile: ignore-conflict.cpp
+Diagnostics:
+  - DiagnosticName: test-ignore-conflict-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/ignore-conflict.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/ignore-conflict.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+  - DiagnosticName: test-ignore-conflict-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/ignore-conflict.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/ignore-conflict.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+...
Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -177,6 +177,7 @@
 def apply_fixes(args, clang_apply_replacements_binary, tmpdir):
   """Calls clang-apply-fixes on a given directory."""
   invocation = [clang_apply_replacements_binary]
+  invocation.append('-ignore-insert-conflict')
   if args.format:
 invocation.append('-format')
   if args.style:
Index: clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
===
--- clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -42,6 +42,11 @@
  "merging/replacing."),
 cl::init(false), cl::cat(ReplacementCategory));
 
+static cl::opt IgnoreInsertConflict(
+"ignore-insert-conflict",
+cl::desc("Ignore insert conflict and keep running to fix."),
+cl::init(false), cl::cat(ReplacementCategory));
+
 static cl::opt DoFormat(
 "format",
 cl::desc("Enable formatting of 

[PATCH] D123924: [clang-apply-replacements] Added support for adjusting conflict offset.

[gehry via Phabricator via cfe-commits]

Sockke created this revision.
Sockke added reviewers: malcolm.parsons, hokein, alexfh.
Herald added a project: All.
Sockke requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.

If two different texts are inserted at the same offset, 
clang-apply-replacements prints the conflict error and discard all fixes. This 
patch adds support for adjusting conflict offset and running to fix them 
continually.

https://godbolt.org/z/P938EGoxj doesn't have any fixes when I run 
run-clang-tidy.py to generate a YAML file with clang-tidy and fix them with 
clang-apply-replacements. The YAML file has two different header texts 
insertions at the same offset, unlike clang-tidy with '-fix', 
clang-apply-replacements does not adjust for this conflict.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123924

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  
clang-tools-extra/test/clang-apply-replacements/Inputs/conflict-offset/conflict-offset.cpp
  
clang-tools-extra/test/clang-apply-replacements/Inputs/conflict-offset/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/conflict-offset.cpp
  clang/include/clang/Tooling/Refactoring/AtomicChange.h


Index: clang/include/clang/Tooling/Refactoring/AtomicChange.h
===
--- clang/include/clang/Tooling/Refactoring/AtomicChange.h
+++ clang/include/clang/Tooling/Refactoring/AtomicChange.h
@@ -116,6 +116,8 @@
   /// Returns a const reference to existing replacements.
   const Replacements () const { return Replaces; }
 
+  Replacements () { return Replaces; }
+
   llvm::ArrayRef getInsertedHeaders() const {
 return InsertedHeaders;
   }
Index: clang-tools-extra/test/clang-apply-replacements/conflict-offset.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/conflict-offset.cpp
@@ -0,0 +1,5 @@
+// RUN: mkdir -p %T/Inputs/conflict-offset
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/conflict-offset/conflict-offset.cpp 
> %T/Inputs/conflict-offset/conflict-offset.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/conflict-offset#" 
%S/Inputs/conflict-offset/file1.yaml > %T/Inputs/conflict-offset/file1.yaml
+// RUN: clang-apply-replacements %T/Inputs/conflict-offset
+// RUN: FileCheck -input-file=%T/Inputs/conflict-offset/conflict-offset.cpp 
%S/Inputs/conflict-offset/conflict-offset.cpp
Index: 
clang-tools-extra/test/clang-apply-replacements/Inputs/conflict-offset/file1.yaml
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-apply-replacements/Inputs/conflict-offset/file1.yaml
@@ -0,0 +1,24 @@
+---
+MainSourceFile: conflict-offset.cpp
+Diagnostics:
+  - DiagnosticName: test-conflict-offset-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/conflict-offset.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/conflict-offset.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+  - DiagnosticName: test-conflict-offset-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/conflict-offset.cpp
+  FileOffset: 0
+  Replacements:
+- FilePath:$(path)/conflict-offset.cpp
+  Offset:  0
+  Length:  0
+  ReplacementText: "#include \n"
+...
Index: 
clang-tools-extra/test/clang-apply-replacements/Inputs/conflict-offset/conflict-offset.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-apply-replacements/Inputs/conflict-offset/conflict-offset.cpp
@@ -0,0 +1,4 @@
+class MyType {};
+// CHECK: #include 
+// CHECK-NEXT: #include 
+// CEHCK-NEXT: class MyType {};
Index: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -231,7 +231,19 @@
 // For now, printing directly the error reported by `AtomicChange` is
 // the easiest solution.
 errs() << llvm::toString(std::move(Err)) << "\n";
-ConflictDetected = true;
+tooling::Replacements  = FileChange.getRefReplacements();
+unsigned NewOffset = 
Replacements.getShiftedCodePosition(R.getOffset());
+unsigned NewLength =
+Replacements.getShiftedCodePosition(R.getOffset() + R.getLength()) 
-
+NewOffset;
+if (NewLength == R.getLength()) {
+  tooling::Replacement RR = tooling::Replacement(
+  R.getFilePath(), NewOffset, NewLength, R.getReplacementText());
+  Replacements = 

[PATCH] D116593: Fix `performance-unnecessary-value-param` for template specialization

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 412340.

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

https://reviews.llvm.org/D116593

Files:
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
@@ -109,7 +109,7 @@
 
 template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-FIXES-NOT: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
@@ -381,3 +381,21 @@
   // CHECK-FIXES: void templateFunction(ExpensiveToCopyType E) {
   E.constReference();
 }
+
+template  T templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:73: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: template  T templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return T();
+}
+
+template <> bool templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:69: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: template <> bool templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return true;
+}
+
+template <> int templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:68: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: template <> int templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return 0;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
@@ -69,7 +69,7 @@
 
 template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-FIXES-NOT: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -116,6 +116,10 @@
   ` when a pure virtual function
   overrided has a const return type. Removed the fix for a virtual function.
 
+- Fixed a crash in :doc:`performance-unnecessary-value-param
+  ` when the specialization
+  template has an unnecessary value paramter. Removed the fix for a template.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -51,18 +51,6 @@
   return Matches.empty();
 }
 
-bool isExplicitTemplateSpecialization(const FunctionDecl ) {
-  if (const auto *SpecializationInfo = Function.getTemplateSpecializationInfo())
-if (SpecializationInfo->getTemplateSpecializationKind() ==
-TSK_ExplicitSpecialization)
-  return true;
-  if (const auto *Method = llvm::dyn_cast())
-if (Method->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization &&
-Method->getMemberSpecializationInfo()->isExplicitSpecialization())
-  return true;
-  return false;
-}
-
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -146,11 +134,12 @@
   // 2. the function is virtual as it might break overrides
   // 3. the function is referenced outside of a call expression within the
   //compilation unit as the signature change could introduce build errors.
-  // 4. the function is an explicit template specialization.
+  // 4. the function is a primary template or an explicit template
+  // specialization.
   const auto *Method = llvm::dyn_cast(Function);
   if (Param->getBeginLoc().isMacroID() || (Method && Method->isVirtual()) ||
   isReferencedOutsideOfCallExpr(*Function, *Result.Context) ||
-  

[PATCH] D116593: Fix `performance-unnecessary-value-param` for template specialization

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 412334.
Herald added a project: All.

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

https://reviews.llvm.org/D116593

Files:
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
@@ -109,7 +109,7 @@
 
 template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-FIXES-NOT: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
@@ -381,3 +381,21 @@
   // CHECK-FIXES: void templateFunction(ExpensiveToCopyType E) {
   E.constReference();
 }
+
+template  T templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:73: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: template  T templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return T();
+}
+
+template <> bool templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:69: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: template <> bool templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return true;
+}
+
+template <> int templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:68: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: template <> int templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return 0;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
@@ -69,7 +69,7 @@
 
 template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-FIXES-NOT: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -116,6 +116,10 @@
   ` related to passing
   arguments that refer to program elements without a trivial identifier.
 
+- Fixed a crash in :doc:`performance-unnecessary-value-param
+  ` when the specialization
+  template has an unnecessary value paramter. Removed the fix for a template.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -51,18 +51,6 @@
   return Matches.empty();
 }
 
-bool isExplicitTemplateSpecialization(const FunctionDecl ) {
-  if (const auto *SpecializationInfo = Function.getTemplateSpecializationInfo())
-if (SpecializationInfo->getTemplateSpecializationKind() ==
-TSK_ExplicitSpecialization)
-  return true;
-  if (const auto *Method = llvm::dyn_cast())
-if (Method->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization &&
-Method->getMemberSpecializationInfo()->isExplicitSpecialization())
-  return true;
-  return false;
-}
-
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -146,11 +134,12 @@
   // 2. the function is virtual as it might break overrides
   // 3. the function is referenced outside of a call expression within the
   //compilation unit as the signature change could introduce build errors.
-  // 4. the function is an explicit template specialization.
+  // 4. the function is a primary template or an explicit template
+  // specialization.
   const auto *Method = llvm::dyn_cast(Function);
   if (Param->getBeginLoc().isMacroID() || (Method && Method->isVirtual()) ||
   isReferencedOutsideOfCallExpr(*Function, 

[PATCH] D116439: [clang-tidy] Fix `readability-const-return-type` for pure virtual function.

[gehry via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGba54ebeb5eba: [clang-tidy] Fix 
`readability-const-return-type` for pure virtual function. (authored by Sockke).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116439

Files:
  clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -271,3 +271,17 @@
 
 int **const * n_multiple_ptr();
 int *const & n_pointer_ref();
+
+class PVBase {
+public:
+  virtual const int getC() = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 
'const'-qualified at the top level, which may reduce code readability without 
improving const correctness
+  // CHECK-NOT-FIXES: virtual int getC() = 0;
+};
+
+class PVDerive : public PVBase {
+public:
+  const int getC() { return 1; }
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 
'const'-qualified at the top level, which may reduce code readability without 
improving const correctness
+  // CHECK-NOT-FIXES: int getC() { return 1; }
+};
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -112,6 +112,10 @@
 - Fixed a false positive in :doc:`readability-non-const-parameter
   ` when the parameter is 
referenced by an lvalue
 
+- Fixed a crash in :doc:`readability-const-return-type
+  ` when a pure virtual 
function
+  overrided has a const return type. Removed the fix for a virtual function.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
@@ -97,7 +97,9 @@
   // Find all function definitions for which the return types are `const`
   // qualified.
   Finder->addMatcher(
-  functionDecl(returns(isConstQualified()), isDefinition()).bind("func"),
+  functionDecl(returns(isConstQualified()),
+   anyOf(isDefinition(), cxxMethodDecl(isPure(
+  .bind("func"),
   this);
 }
 
@@ -115,6 +117,12 @@
 << Def->getReturnType();
 if (CR.ConstRange.isValid())
   Diagnostic << CR.ConstRange;
+
+// Do not propose fixes for virtual function.
+const auto *Method = dyn_cast(Def);
+if (Method && Method->isVirtual())
+  return;
+
 for (auto  : CR.Hints)
   Diagnostic << Hint;
   }


Index: clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -271,3 +271,17 @@
 
 int **const * n_multiple_ptr();
 int *const & n_pointer_ref();
+
+class PVBase {
+public:
+  virtual const int getC() = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+  // CHECK-NOT-FIXES: virtual int getC() = 0;
+};
+
+class PVDerive : public PVBase {
+public:
+  const int getC() { return 1; }
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+  // CHECK-NOT-FIXES: int getC() { return 1; }
+};
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -112,6 +112,10 @@
 - Fixed a false positive in :doc:`readability-non-const-parameter
   ` when the parameter is referenced by an lvalue
 
+- Fixed a crash in :doc:`readability-const-return-type
+  ` when a pure virtual function
+  overrided has a const return type. Removed the fix for a virtual function.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
@@ -97,7 +97,9 @@
 

[PATCH] D116439: [clang-tidy] Fix `readability-const-return-type` for pure virtual function.

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 412056.

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

https://reviews.llvm.org/D116439

Files:
  clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -271,3 +271,17 @@
 
 int **const * n_multiple_ptr();
 int *const & n_pointer_ref();
+
+class PVBase {
+public:
+  virtual const int getC() = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 
'const'-qualified at the top level, which may reduce code readability without 
improving const correctness
+  // CHECK-NOT-FIXES: virtual int getC() = 0;
+};
+
+class PVDerive : public PVBase {
+public:
+  const int getC() { return 1; }
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 
'const'-qualified at the top level, which may reduce code readability without 
improving const correctness
+  // CHECK-NOT-FIXES: int getC() { return 1; }
+};
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -112,6 +112,10 @@
 - Fixed a false positive in :doc:`readability-non-const-parameter
   ` when the parameter is 
referenced by an lvalue
 
+- Fixed a crash in :doc:`readability-const-return-type
+  ` when a pure virtual 
function
+  overrided has a const return type. Removed the fix for a virtual function.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
@@ -97,7 +97,9 @@
   // Find all function definitions for which the return types are `const`
   // qualified.
   Finder->addMatcher(
-  functionDecl(returns(isConstQualified()), isDefinition()).bind("func"),
+  functionDecl(returns(isConstQualified()),
+   anyOf(isDefinition(), cxxMethodDecl(isPure(
+  .bind("func"),
   this);
 }
 
@@ -115,6 +117,12 @@
 << Def->getReturnType();
 if (CR.ConstRange.isValid())
   Diagnostic << CR.ConstRange;
+
+// Do not propose fixes for virtual function.
+const auto *Method = dyn_cast(Def);
+if (Method && Method->isVirtual())
+  return;
+
 for (auto  : CR.Hints)
   Diagnostic << Hint;
   }


Index: clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -271,3 +271,17 @@
 
 int **const * n_multiple_ptr();
 int *const & n_pointer_ref();
+
+class PVBase {
+public:
+  virtual const int getC() = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+  // CHECK-NOT-FIXES: virtual int getC() = 0;
+};
+
+class PVDerive : public PVBase {
+public:
+  const int getC() { return 1; }
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+  // CHECK-NOT-FIXES: int getC() { return 1; }
+};
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -112,6 +112,10 @@
 - Fixed a false positive in :doc:`readability-non-const-parameter
   ` when the parameter is referenced by an lvalue
 
+- Fixed a crash in :doc:`readability-const-return-type
+  ` when a pure virtual function
+  overrided has a const return type. Removed the fix for a virtual function.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
@@ -97,7 +97,9 @@
   // Find all function definitions for which the return types are `const`
   // qualified.
   Finder->addMatcher(
-  functionDecl(returns(isConstQualified()), isDefinition()).bind("func"),
+  functionDecl(returns(isConstQualified()),
+ 

[PATCH] D116593: Fix `performance-unnecessary-value-param` for template specialization

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 411729.
Sockke added a comment.

Removed the fix for a template.


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

https://reviews.llvm.org/D116593

Files:
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
@@ -109,7 +109,7 @@
 
 template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-NOT-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
@@ -381,3 +381,24 @@
   // CHECK-FIXES: void templateFunction(ExpensiveToCopyType E) {
   E.constReference();
 }
+
+template 
+T templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied
+  // CHECK-NOT-FIXES: T templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return T();
+}
+
+template <>
+bool templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:57: warning: the parameter 'E' is copied
+  // CHECK-NOT-FIXES: bool templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return true;
+}
+
+template <>
+int templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:56: warning: the parameter 'E' is copied
+  // CHECK-NOT-FIXES: int templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return 0;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
@@ -69,7 +69,7 @@
 
 template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-NOT-FIXES: template  void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -116,6 +116,10 @@
   ` related to passing
   arguments that refer to program elements without a trivial identifier.
 
+- Fixed a crash in :doc:`performance-unnecessary-value-param
+  ` when the specialization
+  template has an unnecessary value paramter. Removed the fix for a template.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -51,18 +51,6 @@
   return Matches.empty();
 }
 
-bool isExplicitTemplateSpecialization(const FunctionDecl ) {
-  if (const auto *SpecializationInfo = Function.getTemplateSpecializationInfo())
-if (SpecializationInfo->getTemplateSpecializationKind() ==
-TSK_ExplicitSpecialization)
-  return true;
-  if (const auto *Method = llvm::dyn_cast())
-if (Method->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization &&
-Method->getMemberSpecializationInfo()->isExplicitSpecialization())
-  return true;
-  return false;
-}
-
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -146,11 +134,12 @@
   // 2. the function is virtual as it might break overrides
   // 3. the function is referenced outside of a call expression within the
   //compilation unit as the signature change could introduce build errors.
-  // 4. the function is an explicit template specialization.
+  // 4. the function is a primary template or an explicit template
+  // specialization.
   const auto *Method = llvm::dyn_cast(Function);
   if (Param->getBeginLoc().isMacroID() || (Method && Method->isVirtual()) ||
   isReferencedOutsideOfCallExpr(*Function, 

[PATCH] D116439: [clang-tidy] Fix `readability-const-return-type` for pure virtual function.

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 411722.
Sockke added a comment.

Added a release note.


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

https://reviews.llvm.org/D116439

Files:
  clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -271,3 +271,17 @@
 
 int **const * n_multiple_ptr();
 int *const & n_pointer_ref();
+
+class PVBase {
+public:
+  virtual const int getC() = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 
'const'-qualified at the top level, which may reduce code readability without 
improving const correctness
+  // CHECK-NOT-FIXES: virtual int getC() = 0;
+};
+
+class PVDerive : public PVBase {
+public:
+  const int getC() { return 1; }
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 
'const'-qualified at the top level, which may reduce code readability without 
improving const correctness
+  // CHECK-NOT-FIXES: int getC() { return 1; }
+};
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -116,6 +116,10 @@
   ` related to passing
   arguments that refer to program elements without a trivial identifier.
 
+- Fixed a crash in :doc:`readability-const-return-type
+  ` when a pure virtual 
function
+  overrided has a const return type. Removed the fix for a virtual function.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
@@ -97,7 +97,9 @@
   // Find all function definitions for which the return types are `const`
   // qualified.
   Finder->addMatcher(
-  functionDecl(returns(isConstQualified()), isDefinition()).bind("func"),
+  functionDecl(returns(isConstQualified()),
+   anyOf(isDefinition(), cxxMethodDecl(isPure(
+  .bind("func"),
   this);
 }
 
@@ -115,6 +117,12 @@
 << Def->getReturnType();
 if (CR.ConstRange.isValid())
   Diagnostic << CR.ConstRange;
+
+// Do not propose fixes for virtual function.
+const auto *Method = dyn_cast(Def);
+if (Method && Method->isVirtual())
+  return;
+
 for (auto  : CR.Hints)
   Diagnostic << Hint;
   }


Index: clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -271,3 +271,17 @@
 
 int **const * n_multiple_ptr();
 int *const & n_pointer_ref();
+
+class PVBase {
+public:
+  virtual const int getC() = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+  // CHECK-NOT-FIXES: virtual int getC() = 0;
+};
+
+class PVDerive : public PVBase {
+public:
+  const int getC() { return 1; }
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+  // CHECK-NOT-FIXES: int getC() { return 1; }
+};
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -116,6 +116,10 @@
   ` related to passing
   arguments that refer to program elements without a trivial identifier.
 
+- Fixed a crash in :doc:`readability-const-return-type
+  ` when a pure virtual function
+  overrided has a const return type. Removed the fix for a virtual function.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
@@ -97,7 +97,9 @@
   // Find all function definitions for which the return types are `const`
   // qualified.
   Finder->addMatcher(
-  functionDecl(returns(isConstQualified()), isDefinition()).bind("func"),
+  functionDecl(returns(isConstQualified()),
+  

[PATCH] D116439: [clang-tidy] Fix `readability-const-return-type` for pure virtual function.

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 411372.
Sockke added a comment.

Removed the fix for the virtual function.


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

https://reviews.llvm.org/D116439

Files:
  clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -271,3 +271,17 @@
 
 int **const * n_multiple_ptr();
 int *const & n_pointer_ref();
+
+class PVBase {
+public:
+  virtual const int getC() = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 
'const'-qualified at the top level, which may reduce code readability without 
improving const correctness
+  // CHECK-NOT-FIXES: virtual int getC() = 0;
+};
+
+class PVDerive : public PVBase {
+public:
+  const int getC() { return 1; }
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 
'const'-qualified at the top level, which may reduce code readability without 
improving const correctness
+  // CHECK-NOT-FIXES: int getC() { return 1; }
+};
Index: clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
@@ -97,7 +97,9 @@
   // Find all function definitions for which the return types are `const`
   // qualified.
   Finder->addMatcher(
-  functionDecl(returns(isConstQualified()), isDefinition()).bind("func"),
+  functionDecl(returns(isConstQualified()),
+   anyOf(isDefinition(), cxxMethodDecl(isPure(
+  .bind("func"),
   this);
 }
 
@@ -115,6 +117,12 @@
 << Def->getReturnType();
 if (CR.ConstRange.isValid())
   Diagnostic << CR.ConstRange;
+
+// Do not propose fixes for virtual function.
+const auto *Method = llvm::dyn_cast(Def);
+if (Method && Method->isVirtual())
+  return;
+
 for (auto  : CR.Hints)
   Diagnostic << Hint;
   }


Index: clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -271,3 +271,17 @@
 
 int **const * n_multiple_ptr();
 int *const & n_pointer_ref();
+
+class PVBase {
+public:
+  virtual const int getC() = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+  // CHECK-NOT-FIXES: virtual int getC() = 0;
+};
+
+class PVDerive : public PVBase {
+public:
+  const int getC() { return 1; }
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+  // CHECK-NOT-FIXES: int getC() { return 1; }
+};
Index: clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
@@ -97,7 +97,9 @@
   // Find all function definitions for which the return types are `const`
   // qualified.
   Finder->addMatcher(
-  functionDecl(returns(isConstQualified()), isDefinition()).bind("func"),
+  functionDecl(returns(isConstQualified()),
+   anyOf(isDefinition(), cxxMethodDecl(isPure(
+  .bind("func"),
   this);
 }
 
@@ -115,6 +117,12 @@
 << Def->getReturnType();
 if (CR.ConstRange.isValid())
   Diagnostic << CR.ConstRange;
+
+// Do not propose fixes for virtual function.
+const auto *Method = llvm::dyn_cast(Def);
+if (Method && Method->isVirtual())
+  return;
+
 for (auto  : CR.Hints)
   Diagnostic << Hint;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116593: Fix `performance-unnecessary-value-param` for template specialization

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 411336.
Sockke added a comment.

Removed the fix for the template.


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

https://reviews.llvm.org/D116593

Files:
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
@@ -109,7 +109,7 @@
 
 template  void templateWithNonTemplatizedParameter(const 
ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void 
templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-NOT-FIXES: template  void 
templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
@@ -381,3 +381,24 @@
   // CHECK-FIXES: void 
templateFunction(ExpensiveToCopyType E) {
   E.constReference();
 }
+
+template 
+T templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied
+  // CHECK-NOT-FIXES: T templateSpecializationFunction(const 
ExpensiveToCopyType& E) {
+  return T();
+}
+
+template <>
+bool templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:57: warning: the parameter 'E' is copied
+  // CHECK-NOT-FIXES: bool templateSpecializationFunction(const 
ExpensiveToCopyType& E) {
+  return true;
+}
+
+template <>
+int templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:56: warning: the parameter 'E' is copied
+  // CHECK-NOT-FIXES: int templateSpecializationFunction(const 
ExpensiveToCopyType& E) {
+  return 0;
+}
Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp
@@ -69,7 +69,7 @@
 
 template  void templateWithNonTemplatizedParameter(const 
ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template  void 
templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-NOT-FIXES: template  void 
templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -51,18 +51,6 @@
   return Matches.empty();
 }
 
-bool isExplicitTemplateSpecialization(const FunctionDecl ) {
-  if (const auto *SpecializationInfo = 
Function.getTemplateSpecializationInfo())
-if (SpecializationInfo->getTemplateSpecializationKind() ==
-TSK_ExplicitSpecialization)
-  return true;
-  if (const auto *Method = llvm::dyn_cast())
-if (Method->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization &&
-Method->getMemberSpecializationInfo()->isExplicitSpecialization())
-  return true;
-  return false;
-}
-
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -146,11 +134,12 @@
   // 2. the function is virtual as it might break overrides
   // 3. the function is referenced outside of a call expression within the
   //compilation unit as the signature change could introduce build errors.
-  // 4. the function is an explicit template specialization.
+  // 4. the function is a primary template or an explicit template
+  // specialization.
   const auto *Method = llvm::dyn_cast(Function);
   if (Param->getBeginLoc().isMacroID() || (Method && Method->isVirtual()) ||
   isReferencedOutsideOfCallExpr(*Function, *Result.Context) ||
-  isExplicitTemplateSpecialization(*Function))
+  (Function->getTemplatedKind() != FunctionDecl::TK_NonTemplate))
 return;
   for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
FunctionDecl = FunctionDecl->getPreviousDecl()) {


Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
+++ 

[PATCH] D117090: [clang-tidy] Fix `readability-non-const-parameter` for parameter referenced by an lvalue

[gehry via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6cbf15e9b5ac: [clang-tidy] Fix 
`readability-non-const-parameter` for parameter referenced by… (authored by 
Sockke).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117090

Files:
  clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
@@ -287,3 +287,39 @@
 }
 char foo(char *s); // 2
 // CHECK-FIXES: {{^}}char foo(const char *s); // 2{{$}}
+
+void lvalueReference(int *p) {
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  int  = *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:32: warning: pointer parameter 'p' can be
+void constLValueReference(int *p) {
+  // CHECK-FIXES: {{^}}void constLValueReference(const int *p) {{{$}}
+  const int  = *p;
+}
+
+void lambdaLVRef(int *p) {
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  auto foo = [&]() {
+int  = *p;
+  };
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:28: warning: pointer parameter 'p' can be
+void lambdaConstLVRef(int *p) {
+  // CHECK-FIXES: {{^}}void lambdaConstLVRef(const int *p) {{{$}}
+  auto foo = [&]() {
+const int  = *p;
+  };
+}
+
+struct Temp1 {
+  Temp1(int ) {
+i = 10;
+  }
+};
+void constructLVRef(int *p) {
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  Temp1 t(*p);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst
@@ -44,3 +44,8 @@
   int f3(struct S *p) {
 *(p->a) = 0;
   }
+
+  // no warning; p is referenced by an lvalue.
+  void f4(int *p) {
+int  = *p;
+  }
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -109,6 +109,10 @@
 Changes in existing checks
 ^^
 
+- Fixed a false positive in :doc:`readability-non-const-parameter
+  ` when the parameter is referenced by an lvalue
+
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
@@ -83,6 +83,20 @@
   for (const auto *Arg : CE->arguments()) {
 markCanNotBeConst(Arg->IgnoreParenCasts(), true);
   }
+  // Data passed by nonconst reference should not be made const.
+  unsigned ArgNr = 0U;
+  if (const auto *CD = CE->getConstructor()) {
+for (const auto *Par : CD->parameters()) {
+  if (ArgNr >= CE->getNumArgs())
+break;
+  const Expr *Arg = CE->getArg(ArgNr++);
+  // Is this a non constant reference parameter?
+  const Type *ParType = Par->getType().getTypePtr();
+  if (!ParType->isReferenceType() || Par->getType().isConstQualified())
+continue;
+  markCanNotBeConst(Arg->IgnoreParenCasts(), false);
+}
+  }
 } else if (const auto *R = dyn_cast(S)) {
   markCanNotBeConst(R->getRetValue(), true);
 } else if (const auto *U = dyn_cast(S)) {
@@ -93,6 +107,9 @@
 if ((T->isPointerType() && !T->getPointeeType().isConstQualified()) ||
 T->isArrayType())
   markCanNotBeConst(VD->getInit(), true);
+else if (T->isLValueReferenceType() &&
+ !T->getPointeeType().isConstQualified())
+  markCanNotBeConst(VD->getInit(), false);
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117090: [clang-tidy] Fix `readability-non-const-parameter` for parameter referenced by an lvalue

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 411288.
Sockke added a comment.

rebased.


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

https://reviews.llvm.org/D117090

Files:
  clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
@@ -287,3 +287,39 @@
 }
 char foo(char *s); // 2
 // CHECK-FIXES: {{^}}char foo(const char *s); // 2{{$}}
+
+void lvalueReference(int *p) {
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  int  = *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:32: warning: pointer parameter 'p' can be
+void constLValueReference(int *p) {
+  // CHECK-FIXES: {{^}}void constLValueReference(const int *p) {{{$}}
+  const int  = *p;
+}
+
+void lambdaLVRef(int *p) {
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  auto foo = [&]() {
+int  = *p;
+  };
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:28: warning: pointer parameter 'p' can be
+void lambdaConstLVRef(int *p) {
+  // CHECK-FIXES: {{^}}void lambdaConstLVRef(const int *p) {{{$}}
+  auto foo = [&]() {
+const int  = *p;
+  };
+}
+
+struct Temp1 {
+  Temp1(int ) {
+i = 10;
+  }
+};
+void constructLVRef(int *p) {
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  Temp1 t(*p);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst
@@ -44,3 +44,8 @@
   int f3(struct S *p) {
 *(p->a) = 0;
   }
+
+  // no warning; p is referenced by an lvalue.
+  void f4(int *p) {
+int  = *p;
+  }
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -109,6 +109,10 @@
 Changes in existing checks
 ^^
 
+- Fixed a false positive in :doc:`readability-non-const-parameter
+  ` when the parameter is referenced by an lvalue
+
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
@@ -83,6 +83,20 @@
   for (const auto *Arg : CE->arguments()) {
 markCanNotBeConst(Arg->IgnoreParenCasts(), true);
   }
+  // Data passed by nonconst reference should not be made const.
+  unsigned ArgNr = 0U;
+  if (const auto *CD = CE->getConstructor()) {
+for (const auto *Par : CD->parameters()) {
+  if (ArgNr >= CE->getNumArgs())
+break;
+  const Expr *Arg = CE->getArg(ArgNr++);
+  // Is this a non constant reference parameter?
+  const Type *ParType = Par->getType().getTypePtr();
+  if (!ParType->isReferenceType() || Par->getType().isConstQualified())
+continue;
+  markCanNotBeConst(Arg->IgnoreParenCasts(), false);
+}
+  }
 } else if (const auto *R = dyn_cast(S)) {
   markCanNotBeConst(R->getRetValue(), true);
 } else if (const auto *U = dyn_cast(S)) {
@@ -93,6 +107,9 @@
 if ((T->isPointerType() && !T->getPointeeType().isConstQualified()) ||
 T->isArrayType())
   markCanNotBeConst(VD->getInit(), true);
+else if (T->isLValueReferenceType() &&
+ !T->getPointeeType().isConstQualified())
+  markCanNotBeConst(VD->getInit(), false);
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117090: [clang-tidy] Fix `readability-non-const-parameter` for parameter referenced by an lvalue

[gehry via Phabricator via cfe-commits]

Sockke added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp:293
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  int  = *p;
+}

MTC wrote:
> @Sockke  Could you please add the following tests?
> 
> ```
> int  = std::ref(*p);
> const int  = std::ref(*p);
> const int  = std::cref(*p);
> const int *ptr = std::as_const(p);
> int *ptr = const_cast(std::as_const(p));
> decltype(auto) ptr = p;
> auto ptr = p;
> ```
These cases have been handled stably in another logic.


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

https://reviews.llvm.org/D117090

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

[PATCH] D117090: [clang-tidy] Fix `readability-non-const-parameter` for parameter referenced by an lvalue

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 410753.
Sockke added a comment.

Add a release note and improve test description information.


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

https://reviews.llvm.org/D117090

Files:
  clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
@@ -287,3 +287,39 @@
 }
 char foo(char *s); // 2
 // CHECK-FIXES: {{^}}char foo(const char *s); // 2{{$}}
+
+void lvalueReference(int *p) {
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  int  = *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:32: warning: pointer parameter 'p' can be
+void constLValueReference(int *p) {
+  // CHECK-FIXES: {{^}}void constLValueReference(const int *p) {{{$}}
+  const int  = *p;
+}
+
+void lambdaLVRef(int *p) {
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  auto foo = [&]() {
+int  = *p;
+  };
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:28: warning: pointer parameter 'p' can be
+void lambdaConstLVRef(int *p) {
+  // CHECK-FIXES: {{^}}void lambdaConstLVRef(const int *p) {{{$}}
+  auto foo = [&]() {
+const int  = *p;
+  };
+}
+
+struct Temp1 {
+  Temp1(int ) {
+i = 10;
+  }
+};
+void constructLVRef(int *p) {
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  Temp1 t(*p);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst
@@ -44,3 +44,8 @@
   int f3(struct S *p) {
 *(p->a) = 0;
   }
+
+  // no warning; p is referenced by an lvalue.
+  void f4(int *p) {
+int  = *p;
+  }
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -158,6 +158,9 @@
   option to control whether to warn on narrowing integer to floating-point
   conversions.
 
+- Fixed a false positive in :doc:`readability-non-const-parameter
+  ` when the parameter is referenced by an lvalue
+
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
@@ -83,6 +83,20 @@
   for (const auto *Arg : CE->arguments()) {
 markCanNotBeConst(Arg->IgnoreParenCasts(), true);
   }
+  // Data passed by nonconst reference should not be made const.
+  unsigned ArgNr = 0U;
+  if (const auto *CD = CE->getConstructor()) {
+for (const auto *Par : CD->parameters()) {
+  if (ArgNr >= CE->getNumArgs())
+break;
+  const Expr *Arg = CE->getArg(ArgNr++);
+  // Is this a non constant reference parameter?
+  const Type *ParType = Par->getType().getTypePtr();
+  if (!ParType->isReferenceType() || Par->getType().isConstQualified())
+continue;
+  markCanNotBeConst(Arg->IgnoreParenCasts(), false);
+}
+  }
 } else if (const auto *R = dyn_cast(S)) {
   markCanNotBeConst(R->getRetValue(), true);
 } else if (const auto *U = dyn_cast(S)) {
@@ -93,6 +107,9 @@
 if ((T->isPointerType() && !T->getPointeeType().isConstQualified()) ||
 T->isArrayType())
   markCanNotBeConst(VD->getInit(), true);
+else if (T->isLValueReferenceType() &&
+ !T->getPointeeType().isConstQualified())
+  markCanNotBeConst(VD->getInit(), false);
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

In D107450#3257877 , @aaron.ballman 
wrote:

> Do you need someone to commit on your behalf?

Thanks for your review, I committed it myself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107450

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa7f8aea71485: [clang-tidy] Fix wrong FixIt in 
performance-move-const-arg (authored by Sockke).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107450

Files:
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
@@ -246,3 +246,97 @@
   };
   f(MoveSemantics());
 }
+
+void showInt(int &);
+void showInt(int v1, int &);
+void showPointer(const char *&);
+void showPointer2(const char *const &);
+void showTriviallyCopyable(TriviallyCopyable &);
+void showTriviallyCopyablePointer(const TriviallyCopyable *&);
+void testFunctions() {
+  int a = 10;
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-10]]:20: note: consider changing the 1st parameter of 'showInt' from 'int &&' to 'const int &'
+  showInt(int());
+  showInt(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:28: note: consider changing the 2nd parameter of 'showInt' from 'int &&' to 'const int &'
+  const char* s = "";
+  showPointer(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-16]]:32: note: consider changing the 1st parameter of 'showPointer' from 'const char *&&' to 'const char *'
+  showPointer2(std::move(s)); 
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:39: note: consider changing the 1st parameter of 'showPointer2' from 'const char *const &&' to 'const char *const'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  showTriviallyCopyable(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-21]]:48: note: consider changing the 1st parameter of 'showTriviallyCopyable' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+  showTriviallyCopyablePointer(std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-23]]:62: note: consider changing the 1st parameter of 'showTriviallyCopyablePointer' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *'
+}
+template 
+void forwardToShowInt(T && t) {
+  showInt(static_cast(t));
+}
+void testTemplate() {
+  int a = 10;
+  forwardToShowInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+}
+
+struct Tmp {
+  Tmp();
+  Tmp(int &);
+  Tmp(int v1, int &);
+  Tmp(const char *&);
+  Tmp(TriviallyCopyable&& obj);
+  Tmp(const TriviallyCopyable *&);
+  void showTmp(TriviallyCopyable&& t);
+  static void showTmpStatic(TriviallyCopyable&& t);
+};
+void testMethods() {
+  Tmp t;
+  int a = 10;
+  Tmp t1(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:13: note: consider changing the 1st parameter of 'Tmp' from 'int &&' to 'const int &'
+  Tmp t2(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-15]]:21: note: consider changing the 2nd parameter of 'Tmp' from 'int &&' to 'const int &'
+  const char* s = "";
+  Tmp t3(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:21: note: consider changing the 1st parameter of 'Tmp' from 'const char *&&' to 'const char *'
+  

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 401470.
Sockke added a comment.

Improve some description.


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

https://reviews.llvm.org/D107450

Files:
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
@@ -246,3 +246,97 @@
   };
   f(MoveSemantics());
 }
+
+void showInt(int &);
+void showInt(int v1, int &);
+void showPointer(const char *&);
+void showPointer2(const char *const &);
+void showTriviallyCopyable(TriviallyCopyable &);
+void showTriviallyCopyablePointer(const TriviallyCopyable *&);
+void testFunctions() {
+  int a = 10;
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-10]]:20: note: consider changing the 1st parameter of 'showInt' from 'int &&' to 'const int &'
+  showInt(int());
+  showInt(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:28: note: consider changing the 2nd parameter of 'showInt' from 'int &&' to 'const int &'
+  const char* s = "";
+  showPointer(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-16]]:32: note: consider changing the 1st parameter of 'showPointer' from 'const char *&&' to 'const char *'
+  showPointer2(std::move(s)); 
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:39: note: consider changing the 1st parameter of 'showPointer2' from 'const char *const &&' to 'const char *const'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  showTriviallyCopyable(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-21]]:48: note: consider changing the 1st parameter of 'showTriviallyCopyable' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+  showTriviallyCopyablePointer(std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-23]]:62: note: consider changing the 1st parameter of 'showTriviallyCopyablePointer' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *'
+}
+template 
+void forwardToShowInt(T && t) {
+  showInt(static_cast(t));
+}
+void testTemplate() {
+  int a = 10;
+  forwardToShowInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+}
+
+struct Tmp {
+  Tmp();
+  Tmp(int &);
+  Tmp(int v1, int &);
+  Tmp(const char *&);
+  Tmp(TriviallyCopyable&& obj);
+  Tmp(const TriviallyCopyable *&);
+  void showTmp(TriviallyCopyable&& t);
+  static void showTmpStatic(TriviallyCopyable&& t);
+};
+void testMethods() {
+  Tmp t;
+  int a = 10;
+  Tmp t1(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:13: note: consider changing the 1st parameter of 'Tmp' from 'int &&' to 'const int &'
+  Tmp t2(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-15]]:21: note: consider changing the 2nd parameter of 'Tmp' from 'int &&' to 'const int &'
+  const char* s = "";
+  Tmp t3(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:21: note: consider changing the 1st parameter of 'Tmp' from 'const char *&&' to 'const char *'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  Tmp t4(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of 

[PATCH] D115124: [clang-tidy] Fix `readability-container-size-empty` check for smart pointers

[gehry via Phabricator via cfe-commits]

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

I think it looks great and safe enough.  It would be better to turn 
`!(*ptr).empty()` into `!ptr->empty()`, but I have no particular opinions at 
this point.  Let's see if @aaron.ballman has any comments.


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

https://reviews.llvm.org/D115124

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

[PATCH] D117090: [clang-tidy] Fix `readability-non-const-parameter` for parameter referenced by an lvalue

[gehry via Phabricator via cfe-commits]

Sockke added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp:86-99
+  // Data passed by nonconst reference should not be made const.
+  unsigned ArgNr = 0U;
+  if (const auto *CD = CE->getConstructor()) {
+for (const auto *Par : CD->parameters()) {
+  if (ArgNr >= CE->getNumArgs())
+break;
+  const Expr *Arg = CE->getArg(ArgNr++);

MTC wrote:
> `86~99` is pretty close to `64~81`, could you please refactor it?
> `86~99` is pretty close to `64~81`, could you please refactor it?

`FunctionDecl` and `CXXConstructDecl` have different methods with the same name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117090

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

[PATCH] D116593: Fix `performance-unnecessary-value-param` for template specialization

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116593

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

[PATCH] D116439: [clang-tidy] Fix `readability-const-return-type` for pure virtual function.

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping. @aaron.ballman


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116439

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Friendly ping. @aaron.ballman


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

https://reviews.llvm.org/D107450

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

[PATCH] D117090: Fix `readability-non-const-parameter` for parameter referenced by an lvalue

[gehry via Phabricator via cfe-commits]

Sockke created this revision.
Sockke added reviewers: danielmarjamaki, njames93, aaron.ballman, MTC.
Herald added a subscriber: carlosgalvezp.
Sockke requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

The checker missed a check for a case when the parameter is referenced by an 
lvalue and this could cause build breakages.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D117090

Files:
  clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
@@ -287,3 +287,39 @@
 }
 char foo(char *s); // 2
 // CHECK-FIXES: {{^}}char foo(const char *s); // 2{{$}}
+
+void lvalueReference(int *p) {
+  // CHECK-MESSAGES-NOT: warning:
+  int  = *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:32: warning: pointer parameter 'p' can be
+void constLValueReference(int *p) {
+  // CHECK-FIXES: {{^}}void constLValueReference(const int *p) {{{$}}
+  const int  = *p;
+}
+
+void lambdaLVRef(int *p) {
+  // CHECK-MESSAGES-NOT: warning:
+  auto foo = [&]() {
+int  = *p;
+  };
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:28: warning: pointer parameter 'p' can be
+void lambdaConstLVRef(int *p) {
+  // CHECK-FIXES: {{^}}void lambdaConstLVRef(const int *p) {{{$}}
+  auto foo = [&]() {
+const int  = *p;
+  };
+}
+
+struct Temp1 {
+  Temp1(int ) {
+i = 10;
+  }
+};
+void constructLVRef(int *p) {
+  // CHECK-MESSAGES-NOT: warning:
+  Temp1 t(*p);
+}
Index: clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
@@ -83,6 +83,20 @@
   for (const auto *Arg : CE->arguments()) {
 markCanNotBeConst(Arg->IgnoreParenCasts(), true);
   }
+  // Data passed by nonconst reference should not be made const.
+  unsigned ArgNr = 0U;
+  if (const auto *CD = CE->getConstructor()) {
+for (const auto *Par : CD->parameters()) {
+  if (ArgNr >= CE->getNumArgs())
+break;
+  const Expr *Arg = CE->getArg(ArgNr++);
+  // Is this a non constant reference parameter?
+  const Type *ParType = Par->getType().getTypePtr();
+  if (!ParType->isReferenceType() || Par->getType().isConstQualified())
+continue;
+  markCanNotBeConst(Arg->IgnoreParenCasts(), false);
+}
+  }
 } else if (const auto *R = dyn_cast(S)) {
   markCanNotBeConst(R->getRetValue(), true);
 } else if (const auto *U = dyn_cast(S)) {
@@ -93,6 +107,9 @@
 if ((T->isPointerType() && !T->getPointeeType().isConstQualified()) ||
 T->isArrayType())
   markCanNotBeConst(VD->getInit(), true);
+else if (T->isLValueReferenceType() &&
+ !T->getPointeeType().isConstQualified())
+  markCanNotBeConst(VD->getInit(), false);
   }
 }
 


Index: clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp
@@ -287,3 +287,39 @@
 }
 char foo(char *s); // 2
 // CHECK-FIXES: {{^}}char foo(const char *s); // 2{{$}}
+
+void lvalueReference(int *p) {
+  // CHECK-MESSAGES-NOT: warning:
+  int  = *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:32: warning: pointer parameter 'p' can be
+void constLValueReference(int *p) {
+  // CHECK-FIXES: {{^}}void constLValueReference(const int *p) {{{$}}
+  const int  = *p;
+}
+
+void lambdaLVRef(int *p) {
+  // CHECK-MESSAGES-NOT: warning:
+  auto foo = [&]() {
+int  = *p;
+  };
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:28: warning: pointer parameter 'p' can be
+void lambdaConstLVRef(int *p) {
+  // CHECK-FIXES: {{^}}void lambdaConstLVRef(const int *p) {{{$}}
+  auto foo = [&]() {
+const int  = *p;
+  };
+}
+
+struct Temp1 {
+  Temp1(int ) {
+i = 10;
+  }
+};
+void constructLVRef(int *p) {
+  // CHECK-MESSAGES-NOT: warning:
+  Temp1 t(*p);
+}
Index: clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
@@ -83,6 +83,20 @@
   for (const auto *Arg : CE->arguments()) {
 markCanNotBeConst(Arg->IgnoreParenCasts(), true);
   }
+  // Data passed 

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added inline comments.



Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:161
+  // Generate notes for an invocation with an rvalue reference parameter.
+  const auto *ReceivingCallExpr = dyn_cast(ReceivingExpr);
+  const auto *ReceivingConstructExpr =

`ReceivingExpr` is not null if `IsRVRefParam` is true.



Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:165
+  // Skipping the invocation which is a template instantiation.
+  if ((!ReceivingCallExpr || !ReceivingCallExpr->getDirectCallee() ||
+   ReceivingCallExpr->getDirectCallee()->isTemplateInstantiation()) &&

Add a check for `getDirectCallee()`.



Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:175
+  ReceivingCallExpr
+  ? ReceivingCallExpr->getDirectCallee()->getUnderlyingDecl()
+  : ReceivingConstructExpr->getConstructor()->getUnderlyingDecl();

I have added a check for `getDirectCallee` before here.


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

https://reviews.llvm.org/D107450

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 398077.
Sockke marked an inline comment as done.
Sockke added a comment.

Add some checks for `null` and comments for codes.


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

https://reviews.llvm.org/D107450

Files:
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
@@ -246,3 +246,97 @@
   };
   f(MoveSemantics());
 }
+
+void showInt(int &);
+void showInt(int v1, int &);
+void showPointer(const char *&);
+void showPointer2(const char *const &);
+void showTriviallyCopyable(TriviallyCopyable &);
+void showTriviallyCopyablePointer(const TriviallyCopyable *&);
+void testFunctions() {
+  int a = 10;
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-10]]:20: note: consider changing the 1st parameter of 'showInt' from 'int &&' to 'const int &'
+  showInt(int());
+  showInt(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:28: note: consider changing the 2nd parameter of 'showInt' from 'int &&' to 'const int &'
+  const char* s = "";
+  showPointer(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-16]]:32: note: consider changing the 1st parameter of 'showPointer' from 'const char *&&' to 'const char *'
+  showPointer2(std::move(s)); 
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:39: note: consider changing the 1st parameter of 'showPointer2' from 'const char *const &&' to 'const char *const'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  showTriviallyCopyable(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-21]]:48: note: consider changing the 1st parameter of 'showTriviallyCopyable' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+  showTriviallyCopyablePointer(std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-23]]:62: note: consider changing the 1st parameter of 'showTriviallyCopyablePointer' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *'
+}
+template 
+void forwardToShowInt(T && t) {
+  showInt(static_cast(t));
+}
+void testTemplate() {
+  int a = 10;
+  forwardToShowInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+}
+
+struct Tmp {
+  Tmp();
+  Tmp(int &);
+  Tmp(int v1, int &);
+  Tmp(const char *&);
+  Tmp(TriviallyCopyable&& obj);
+  Tmp(const TriviallyCopyable *&);
+  void showTmp(TriviallyCopyable&& t);
+  static void showTmpStatic(TriviallyCopyable&& t);
+};
+void testMethods() {
+  Tmp t;
+  int a = 10;
+  Tmp t1(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:13: note: consider changing the 1st parameter of 'Tmp' from 'int &&' to 'const int &'
+  Tmp t2(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-15]]:21: note: consider changing the 2nd parameter of 'Tmp' from 'int &&' to 'const int &'
+  const char* s = "";
+  Tmp t3(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:21: note: consider changing the 1st parameter of 'Tmp' from 'const char *&&' to 'const char *'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  Tmp 

[PATCH] D116593: Fix `performance-unnecessary-value-param` for template specialization

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Thanks for your review!




Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp:177-178
 const auto  = *FunctionDecl->getParamDecl(Index);
+if (IsExplicitTemplateSpecialization && Function != FunctionDecl)
+  continue;
 Diag << utils::fixit::changeVarDeclToReference(CurrentParam,

flx wrote:
> Could you add a comment here why we're skipping the fix here?
> Could you add a comment here why we're skipping the fix here?

Specialization template may match the primary template again by 
`getPreviousDecl`. Skipping the fix to avoid repeated fixes for the primary 
template.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp:388
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied
+  // CHECK-FIXES: T templateSpecializationFunction(const ExpensiveToCopyType& 
E) {
+  return T();

flx wrote:
> Should we apply the fixes or just issue the warning? For virtual methods we 
> suppress the fix since we can't necessarily update all overrides of the 
> method. Are template specializations always guaranteed to be in the same 
> translation unit which  would make this safe?
> Should we apply the fixes or just issue the warning? For virtual methods we 
> suppress the fix since we can't necessarily update all overrides of the 
> method. Are template specializations always guaranteed to be in the same 
> translation unit which  would make this safe?

Do you mean that specialization templates are defined in different translation 
units?  If fixing one by one translation unit does have the problem, the 
`readability-const-return-type` also has such a problem. clang-tidy can not 
analyze across translation units, but the diagnosis and fix of it are separate, 
we can specify the complete `compile_commands.json` to avoid it. 
I'm not sure whether this is reasonable, we may make a choice between 
clang-tidy's fault tolerance and advantages. What's your suggestion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116593

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

[PATCH] D116593: Fix `performance-unnecessary-value-param` for template specialization

[gehry via Phabricator via cfe-commits]

Sockke created this revision.
Sockke added reviewers: aaron.ballman, flx, njames93.
Herald added a subscriber: carlosgalvezp.
Sockke requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

The checker missed a check for parameter type of primary template of 
specialization template and this could cause build breakages.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116593

Files:
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
@@ -381,3 +381,24 @@
   // CHECK-FIXES: void 
templateFunction(ExpensiveToCopyType E) {
   E.constReference();
 }
+
+template 
+T templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied
+  // CHECK-FIXES: T templateSpecializationFunction(const ExpensiveToCopyType& 
E) {
+  return T();
+}
+
+template <>
+bool templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:57: warning: the parameter 'E' is copied
+  // CHECK-FIXES: bool templateSpecializationFunction(const 
ExpensiveToCopyType& E) {
+  return true;
+}
+
+template <>
+int templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:56: warning: the parameter 'E' is copied
+  // CHECK-FIXES: int templateSpecializationFunction(const 
ExpensiveToCopyType& E) {
+  return 0;
+}
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -51,6 +51,21 @@
   return Matches.empty();
 }
 
+bool isTemplateTypeParmOfPrimaryTemplate(const FunctionDecl ,
+ int Index) {
+  // Check the specific parameter type of primary template whether is template
+  // type.
+  if (const auto *PrimaryTemplateInfo = Function.getPrimaryTemplate()) {
+if (const auto *PrimaryTemplateFunction =
+PrimaryTemplateInfo->getTemplatedDecl()) {
+  const auto *ParamInfo = PrimaryTemplateFunction->getParamDecl(Index);
+  if (ParamInfo && ParamInfo->getOriginalType()->isTemplateTypeParmType())
+return true;
+}
+  }
+  return false;
+}
+
 bool isExplicitTemplateSpecialization(const FunctionDecl ) {
   if (const auto *SpecializationInfo = 
Function.getTemplateSpecializationInfo())
 if (SpecializationInfo->getTemplateSpecializationKind() ==
@@ -146,15 +161,21 @@
   // 2. the function is virtual as it might break overrides
   // 3. the function is referenced outside of a call expression within the
   //compilation unit as the signature change could introduce build errors.
-  // 4. the function is an explicit template specialization.
+  // 4. the function is an explicit template specialization and the specific
+  //parameter of primary template is template type.
   const auto *Method = llvm::dyn_cast(Function);
+  bool IsExplicitTemplateSpecialization =
+  isExplicitTemplateSpecialization(*Function);
   if (Param->getBeginLoc().isMacroID() || (Method && Method->isVirtual()) ||
   isReferencedOutsideOfCallExpr(*Function, *Result.Context) ||
-  isExplicitTemplateSpecialization(*Function))
+  (IsExplicitTemplateSpecialization &&
+   isTemplateTypeParmOfPrimaryTemplate(*Function, Index)))
 return;
   for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
FunctionDecl = FunctionDecl->getPreviousDecl()) {
 const auto  = *FunctionDecl->getParamDecl(Index);
+if (IsExplicitTemplateSpecialization && Function != FunctionDecl)
+  continue;
 Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
*Result.Context);
 // The parameter of each declaration needs to be checked individually as to


Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
@@ -381,3 +381,24 @@
   // CHECK-FIXES: void templateFunction(ExpensiveToCopyType E) {
   E.constReference();
 }
+
+template 
+T templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied
+  // 

[PATCH] D116439: Fix `readability-const-return-type` for pure virtual function.

[gehry via Phabricator via cfe-commits]

Sockke created this revision.
Sockke added a reviewer: aaron.ballman.
Herald added a subscriber: carlosgalvezp.
Sockke requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

It cannot match a `pure virtual function`. This patch fixes this behavior.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116439

Files:
  clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -271,3 +271,17 @@
 
 int **const * n_multiple_ptr();
 int *const & n_pointer_ref();
+
+class PVBase {
+public:
+  virtual const int getC() = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 
'const'-qualified at the top level, which may reduce code readability without 
improving const correctness
+  // CHECK-FIXES: virtual int getC() = 0;
+};
+
+class PVDerive : public PVBase {
+public:
+  const int getC() { return 1; }
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 
'const'-qualified at the top level, which may reduce code readability without 
improving const correctness
+  // CHECK-FIXES: int getC() { return 1; }
+};
Index: clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
@@ -97,7 +97,9 @@
   // Find all function definitions for which the return types are `const`
   // qualified.
   Finder->addMatcher(
-  functionDecl(returns(isConstQualified()), isDefinition()).bind("func"),
+  functionDecl(returns(isConstQualified()),
+   anyOf(isDefinition(), cxxMethodDecl(isPure(
+  .bind("func"),
   this);
 }
 


Index: clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -271,3 +271,17 @@
 
 int **const * n_multiple_ptr();
 int *const & n_pointer_ref();
+
+class PVBase {
+public:
+  virtual const int getC() = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+  // CHECK-FIXES: virtual int getC() = 0;
+};
+
+class PVDerive : public PVBase {
+public:
+  const int getC() { return 1; }
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+  // CHECK-FIXES: int getC() { return 1; }
+};
Index: clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
@@ -97,7 +97,9 @@
   // Find all function definitions for which the return types are `const`
   // qualified.
   Finder->addMatcher(
-  functionDecl(returns(isConstQualified()), isDefinition()).bind("func"),
+  functionDecl(returns(isConstQualified()),
+   anyOf(isDefinition(), cxxMethodDecl(isPure(
+  .bind("func"),
   this);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke marked 13 inline comments as done.
Sockke added inline comments.



Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:145
+  if ((!ReceivingCallExpr ||
+   ReceivingCallExpr->getDirectCallee()->isTemplateInstantiation()) &&
+  (!ReceivingConstructExpr ||

aaron.ballman wrote:
> Not all `CallExpr` objects have a direct callee, so this will crash (such as 
> calls through a function pointer rather than a direct function call). It may 
> be worth adding test coverage for this.
Use `InvocationParm` to determine whether objects have a direct callee.


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

https://reviews.llvm.org/D107450

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 396385.
Sockke added a comment.

Sorry for the late reply. I made improvements to the patch in response to the 
questions you raised. @aaron.ballman


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

https://reviews.llvm.org/D107450

Files:
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
@@ -246,3 +246,97 @@
   };
   f(MoveSemantics());
 }
+
+void showInt(int &);
+void showInt(int v1, int &);
+void showPointer(const char *&);
+void showPointer2(const char *const &);
+void showTriviallyCopyable(TriviallyCopyable &);
+void showTriviallyCopyablePointer(const TriviallyCopyable *&);
+void testFunctions() {
+  int a = 10;
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-10]]:20: note: consider changing the 1st parameter of 'showInt' from 'int &&' to 'const int &'
+  showInt(int());
+  showInt(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:28: note: consider changing the 2nd parameter of 'showInt' from 'int &&' to 'const int &'
+  const char* s = "";
+  showPointer(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-16]]:32: note: consider changing the 1st parameter of 'showPointer' from 'const char *&&' to 'const char *'
+  showPointer2(std::move(s)); 
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:39: note: consider changing the 1st parameter of 'showPointer2' from 'const char *const &&' to 'const char *const'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  showTriviallyCopyable(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-21]]:48: note: consider changing the 1st parameter of 'showTriviallyCopyable' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+  showTriviallyCopyablePointer(std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-23]]:62: note: consider changing the 1st parameter of 'showTriviallyCopyablePointer' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *'
+}
+template 
+void forwardToShowInt(T && t) {
+  showInt(static_cast(t));
+}
+void testTemplate() {
+  int a = 10;
+  forwardToShowInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+}
+
+struct Tmp {
+  Tmp();
+  Tmp(int &);
+  Tmp(int v1, int &);
+  Tmp(const char *&);
+  Tmp(TriviallyCopyable&& obj);
+  Tmp(const TriviallyCopyable *&);
+  void showTmp(TriviallyCopyable&& t);
+  static void showTmpStatic(TriviallyCopyable&& t);
+};
+void testMethods() {
+  Tmp t;
+  int a = 10;
+  Tmp t1(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:13: note: consider changing the 1st parameter of 'Tmp' from 'int &&' to 'const int &'
+  Tmp t2(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-15]]:21: note: consider changing the 2nd parameter of 'Tmp' from 'int &&' to 'const int &'
+  const char* s = "";
+  Tmp t3(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:21: note: consider changing the 1st parameter of 'Tmp' from 'const char *&&' to 'const char *'
+  TriviallyCopyable *obj = new TriviallyCopyable();

[PATCH] D115124: [clang-tidy] Fix `readability-container-size-empty` check for smart pointers

[gehry via Phabricator via cfe-commits]

Sockke requested changes to this revision.
Sockke added a comment.
This revision now requires changes to proceed.

Could you please add a test case where the smart pointer object is dereferenced 
before calling `size()`? E.g. `return (*ptr).size() == 0;`. I think this change 
doesn't work on this test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115124

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Kindly ping. @aaron.ballman


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

https://reviews.llvm.org/D107450

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

In D107450#3155019 , @whisperity 
wrote:

> This generally looks fine for me, but I'd rather have other people who are 
> more knowledgeable about the standard's intricacies looked at it too.
>
> AFAIK Aaron is busy this week and the next (?) due to C++ Committee meetings, 
> or something like that. And I bet after such meetings everyone would take a 
> few more days off from looking at C++ stuff.

Ok, that sounds great, thanks a lot!


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

https://reviews.llvm.org/D107450

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Hi, Could anyone please review this diff? @whisperity, @aaron.ballman


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

https://reviews.llvm.org/D107450

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Kindly ping. @whisperity


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

https://reviews.llvm.org/D107450

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Kindly ping. @whisperity


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

https://reviews.llvm.org/D107450

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

[PATCH] D113148: Add new clang-tidy check for string_view(nullptr)

[gehry via Phabricator via cfe-commits]

Sockke added subscribers: whisperity, aaron.ballman.
Sockke added a comment.

In D113148#3108705 , @CJ-Johnson 
wrote:

> In D113148#3107897 , @Sockke wrote:
>
>> This seems to be an existing check. Have you compared it with 
>> **bugprone-string-constructor**?
>
> Thanks for the suggestion! From what I can tell, bugprone-string-constructor 
> check only has warnings and does not provide fixes in most cases. The goal of 
> bugprone-stringview-nullptr is to robustly enumerate the many cases that it 
> cares about and provide fixes. For that reason, I think making it a separate 
> check is best.

Yes, But i think that improving existing check is the best way. Because 
improving bugprone-string-construct in a new check may make developers confused 
and cause redundant overlap.
Let's see if @aaron.ballman or @whisperity has any comments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113148

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 384728.
Sockke added a comment.

In addition, the splicing logics of "FunctionName" and "ExpectParmTypeName" 
were improved.


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

https://reviews.llvm.org/D107450

Files:
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
@@ -246,3 +246,87 @@
   };
   f(MoveSemantics());
 }
+
+void showInt(int &);
+void showInt(int v1, int &);
+void showPointer(const char *&);
+void showPointer2(const char *const &);
+void showTriviallyCopyable(TriviallyCopyable &);
+void showTriviallyCopyablePointer(const TriviallyCopyable *&);
+void testFunctions() {
+  int a = 10;
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-10]]:20: note: consider changing the 1st parameter of showInt from 'int &&' to 'const int &'
+  showInt(int());
+  showInt(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:28: note: consider changing the 2nd parameter of showInt from 'int &&' to 'const int &'
+  const char* s = "";
+  showPointer(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-16]]:32: note: consider changing the 1st parameter of showPointer from 'const char *&&' to 'const char *'
+  showPointer2(std::move(s)); 
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:39: note: consider changing the 1st parameter of showPointer2 from 'const char *const &&' to 'const char *const'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  showTriviallyCopyable(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-21]]:48: note: consider changing the 1st parameter of showTriviallyCopyable from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+  showTriviallyCopyablePointer(std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-23]]:62: note: consider changing the 1st parameter of showTriviallyCopyablePointer from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *'
+}
+template 
+void forwardToShowInt(T && t) {
+  showInt(static_cast(t));
+}
+void testTemplate() {
+  int a = 10;
+  forwardToShowInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+}
+
+struct Tmp {
+  Tmp();
+  Tmp(int &);
+  Tmp(int v1, int &);
+  Tmp(const char *&);
+  Tmp(TriviallyCopyable&& obj);
+  Tmp(const TriviallyCopyable *&);
+  void showTmp(TriviallyCopyable&& t);
+  static void showTmpStatic(TriviallyCopyable&& t);
+};
+void testMethods() {
+  Tmp t;
+  int a = 10;
+  Tmp t1(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:13: note: consider changing the 1st parameter of Tmp from 'int &&' to 'const int &'
+  Tmp t2(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-15]]:21: note: consider changing the 2nd parameter of Tmp from 'int &&' to 'const int &'
+  const char* s = "";
+  Tmp t3(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:21: note: consider changing the 1st parameter of Tmp from 'const char *&&' to 'const char *'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  Tmp t4(std::move(*obj));
+  // 

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 384706.
Sockke added a comment.

I improved the logic of judgments needing to report suggesting notes in some 
cases. And more tests were added here.


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

https://reviews.llvm.org/D107450

Files:
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
@@ -246,3 +246,87 @@
   };
   f(MoveSemantics());
 }
+
+void showInt(int &);
+void showInt(int v1, int &);
+void showPointer(const char *&);
+void showPointer2(const char *const &);
+void showTriviallyCopyable(TriviallyCopyable &);
+void showTriviallyCopyablePointer(const TriviallyCopyable *&);
+void testFunctions() {
+  int a = 10;
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-10]]:20: note: consider changing the 1st parameter of showInt from 'int &&' to 'const int &'
+  showInt(int());
+  showInt(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:28: note: consider changing the 2nd parameter of showInt from 'int &&' to 'const int &'
+  const char* s = "";
+  showPointer(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-16]]:32: note: consider changing the 1st parameter of showPointer from 'const char *&&' to 'const char *'
+  showPointer2(std::move(s)); 
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:39: note: consider changing the 1st parameter of showPointer2 from 'const char *const &&' to 'const char *const'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  showTriviallyCopyable(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-21]]:48: note: consider changing the 1st parameter of showTriviallyCopyable from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+  showTriviallyCopyablePointer(std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-23]]:62: note: consider changing the 1st parameter of showTriviallyCopyablePointer from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *'
+}
+template 
+void forwardToShowInt(T && t) {
+  showInt(static_cast(t));
+}
+void testTemplate() {
+  int a = 10;
+  forwardToShowInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+}
+
+struct Tmp {
+  Tmp();
+  Tmp(int &);
+  Tmp(int v1, int &);
+  Tmp(const char *&);
+  Tmp(TriviallyCopyable&& obj);
+  Tmp(const TriviallyCopyable *&);
+  void showTmp(TriviallyCopyable&& t);
+  static void showTmpStatic(TriviallyCopyable&& t);
+};
+void testMethods() {
+  Tmp t;
+  int a = 10;
+  Tmp t1(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:13: note: consider changing the 1st parameter of Tmp from 'int &&' to 'const int &'
+  Tmp t2(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-15]]:21: note: consider changing the 2nd parameter of Tmp from 'int &&' to 'const int &'
+  const char* s = "";
+  Tmp t3(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:21: note: consider changing the 1st parameter of Tmp from 'const char *&&' to 'const char *'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  Tmp 

[PATCH] D113148: Add new clang-tidy check for string_view(nullptr)

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

This seems to be an existing check. Have you compared it with 
**bugprone-string-constructor**?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113148

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

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

In D107450#3087103 , @whisperity 
wrote:

> I think this is becoming okay and looks safe enough. I can't really grasp 
> what `HasCheckedMoveSet` means, and how it synergises with storing 
> `CallExpr`s so maybe a better name should be added. Do you mean 
> `AlreadyCheckedMoves`? When is it possible that the same `CallExpr` of 
> `std::move` will be matched multiple times?

Yes, you're right!


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

https://reviews.llvm.org/D107450

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 382632.
Sockke added a comment.

Update!


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

https://reviews.llvm.org/D107450

Files:
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
@@ -246,3 +246,40 @@
   };
   f(MoveSemantics());
 }
+
+void showInt(int &) {}
+void showInt(int v1, int &) {}
+void testInt() {
+  int a = 10;
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-6]]:20: note: consider changing the 1st parameter of showInt from 'int &&' to 'const int &'
+  showInt(int());
+  showInt(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-9]]:28: note: consider changing the 2nd parameter of showInt from 'int &&' to 'const int &'
+}
+template 
+void forwardToShowInt(T && t) {
+  showInt(static_cast(t));
+}
+void testTemplate() {
+  int a = 10;
+  forwardToShowInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+}
+
+struct Tmp {};
+void showTmp(Tmp &) {}
+void testTmp() {
+  Tmp t;
+  showTmp(std::move(t));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 't' of the trivially-copyable type 'Tmp' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-5]]:20: note: consider changing the 1st parameter of showTmp from 'Tmp &&' to 'const Tmp &'
+}
+
+void showA(A && v) {}
+void testA() {
+  A a;
+  showA(std::move(a));
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -113,6 +113,10 @@
 Changes in existing checks
 ^^
 
+- Improved :doc:`performance-move-const-arg` check.
+
+  Removed fixes for that a trivially-copyable object wrapped by std::move is passed to the function with rvalue reference parameters because the code was broken by removing std::move.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
===
--- clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
+++ clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTANTARGUMENTCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "llvm/ADT/DenseSet.h"
 
 namespace clang {
 namespace tidy {
@@ -36,6 +37,7 @@
 
 private:
   const bool CheckTriviallyCopyableMove;
+  llvm::DenseSet AlreadyCheckedMoves;
 };
 
 } // namespace performance
Index: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
@@ -50,7 +50,9 @@
   Finder->addMatcher(
   invocation(forEachArgumentWithParam(
  MoveCallMatcher,
- parmVarDecl(hasType(references(isConstQualified())
+ parmVarDecl(anyOf(hasType(references(isConstQualified())),
+   hasType(rValueReferenceType(
+ .bind("invocation-parm")))
   .bind("receiving-expr"),
   this);
 }
@@ -58,6 +60,15 @@
 void MoveConstArgCheck::check(const MatchFinder::MatchResult ) {
   const auto *CallMove = Result.Nodes.getNodeAs("call-move");
   const auto *ReceivingExpr = Result.Nodes.getNodeAs("receiving-expr");
+  const auto *InvocationParm =
+  Result.Nodes.getNodeAs("invocation-parm");
+
+  if (!ReceivingExpr && AlreadyCheckedMoves.contains(CallMove))
+return;
+
+  if (ReceivingExpr)
+AlreadyCheckedMoves.insert(CallMove);
+
   const Expr *Arg = CallMove->getArg(0);
   SourceManager  = Result.Context->getSourceManager();
 
@@ -90,20 +101,53 @@
   return;
 
 bool IsVariable = isa(Arg);
+bool IsRValueReferenceArg = false;
 const auto *Var =
 IsVariable ? dyn_cast(Arg)->getDecl() : nullptr;
-auto Diag = diag(FileMoveRange.getBegin(),
- 

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added a comment.
Herald added a subscriber: carlosgalvezp.

Kindly ping. @whisperity, @aaron.ballman, @Quuxplusone


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

https://reviews.llvm.org/D107450

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Hi, Could anyone please review this diff? @whisperity, @aaron.ballman


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

https://reviews.llvm.org/D107450

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke marked 12 inline comments as done.
Sockke added a comment.

Kindly ping.


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

https://reviews.llvm.org/D107450

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

[PATCH] D111625: [clang-tidy] bugprone-argument-comment: SourceLocation valid judgment avoid emitting coredump in isInSystemHeader

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

In D111625#3060978 , @aaron.ballman 
wrote:

> LGTM, thank you! Do you need me to commit on your behalf? I'm happy to do so, 
> but given the number of quality submissions you've had, I'm wondering if 
> you'd like to obtain commit access of your own? 
> (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access has the 
> details of what that entails.)

Thanks a lot! It's my pleasure to obtain commit access of my own. I will send 
the application as required later. Before that, I would appreciate it if you 
could commit it on my behalf. Thanks again! 
Name: liuke
Email: liuke.ge...@bytedance.com


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

https://reviews.llvm.org/D111625

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

[PATCH] D111625: [clang-tidy] bugprone-argument-comment: SourceLocation valid judgment avoid emitting coredump in isInSystemHeader

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 379296.
Sockke edited the summary of this revision.
Sockke added a reviewer: MTC.
Sockke added a comment.

Update!


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

https://reviews.llvm.org/D111625

Files:
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
@@ -151,3 +151,8 @@
   my_system_header_function(/*not_arg=*/1);
 }
 } // namespace system_header
+
+void testInvalidSlocCxxConstructExpr() {
+  __builtin_va_list __args;
+  // __builtin_va_list has no defination in any source file
+} 
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -24,6 +24,8 @@
   if (const auto *D = Node.getDeclContext()->getEnclosingNamespaceContext())
 if (D->isStdNamespace())
   return true;
+  if (Node.getLocation().isInvalid())
+return false;
   return Node.getASTContext().getSourceManager().isInSystemHeader(
   Node.getLocation());
 }


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
@@ -151,3 +151,8 @@
   my_system_header_function(/*not_arg=*/1);
 }
 } // namespace system_header
+
+void testInvalidSlocCxxConstructExpr() {
+  __builtin_va_list __args;
+  // __builtin_va_list has no defination in any source file
+} 
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -24,6 +24,8 @@
   if (const auto *D = Node.getDeclContext()->getEnclosingNamespaceContext())
 if (D->isStdNamespace())
   return true;
+  if (Node.getLocation().isInvalid())
+return false;
   return Node.getASTContext().getSourceManager().isInSystemHeader(
   Node.getLocation());
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111625: [clang-tidy] bugprone-argument-comment: SourceLocation valid judgment avoid emitting coredump in isInSystemHeader

[gehry via Phabricator via cfe-commits]

Sockke created this revision.
Sockke added reviewers: aaron.ballman, njames93, george.burgess.iv, hokein.
Herald added a subscriber: xazax.hun.
Sockke requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

If the Node has an invalid location, it will trigger assert in 
isInSystemHeader(...).

  #include 
  /*
...
  */

invalid sloc like "__va_list_tag"
coredump with "Assertion `Loc.isValid() && "Can't get file characteristic of 
invalid loc!"' failed." in getFileCharacteristic(SourceLocation)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D111625

Files:
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp


Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -24,6 +24,8 @@
   if (const auto *D = Node.getDeclContext()->getEnclosingNamespaceContext())
 if (D->isStdNamespace())
   return true;
+  if (Node.getLocation().isInvalid())
+return false;
   return Node.getASTContext().getSourceManager().isInSystemHeader(
   Node.getLocation());
 }


Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -24,6 +24,8 @@
   if (const auto *D = Node.getDeclContext()->getEnclosingNamespaceContext())
 if (D->isStdNamespace())
   return true;
+  if (Node.getLocation().isInvalid())
+return false;
   return Node.getASTContext().getSourceManager().isInSystemHeader(
   Node.getLocation());
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 378168.
Sockke retitled this revision from "[clang-tidy] Fix wrong FIxIt in 
performance-move-const-arg" to "[clang-tidy] Fix wrong FixIt in 
performance-move-const-arg".
Sockke added a comment.

Sorry for the delayed reply because of National Day.  I have updated. 
@whisperity


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

https://reviews.llvm.org/D107450

Files:
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
  clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
  clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
@@ -246,3 +246,40 @@
   };
   f(MoveSemantics());
 }
+
+void showInt(int &) {}
+void showInt(int v1, int &) {}
+void testInt() {
+  int a = 10;
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-6]]:20: note: consider changing the 1st parameter of showInt from 'int &&' to 'const int &'
+  showInt(int());
+  showInt(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-9]]:28: note: consider changing the 2st parameter of showInt from 'int &&' to 'const int &'
+}
+template 
+void forwardToShowInt(T && t) {
+  showInt(static_cast(t));
+}
+void testTemplate() {
+  int a = 10;
+  forwardToShowInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+}
+
+struct Tmp {};
+void showTmp(Tmp &) {}
+void testTmp() {
+  Tmp t;
+  showTmp(std::move(t));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 't' of the trivially-copyable type 'Tmp' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-5]]:20: note: consider changing the 1st parameter of showTmp from 'Tmp &&' to 'const Tmp &'
+}
+
+void showA(A && v) {}
+void testA() {
+  A a;
+  showA(std::move(a));
+}
Index: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
===
--- clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
+++ clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTANTARGUMENTCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "llvm/ADT/DenseSet.h"
 
 namespace clang {
 namespace tidy {
@@ -36,6 +37,7 @@
 
 private:
   const bool CheckTriviallyCopyableMove;
+  llvm::DenseSet HasCheckedMoveSet;
 };
 
 } // namespace performance
Index: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
@@ -50,7 +50,9 @@
   Finder->addMatcher(
   invocation(forEachArgumentWithParam(
  MoveCallMatcher,
- parmVarDecl(hasType(references(isConstQualified())
+ parmVarDecl(anyOf(hasType(references(isConstQualified())),
+   hasType(rValueReferenceType(
+ .bind("invocation-parm")))
   .bind("receiving-expr"),
   this);
 }
@@ -58,6 +60,15 @@
 void MoveConstArgCheck::check(const MatchFinder::MatchResult ) {
   const auto *CallMove = Result.Nodes.getNodeAs("call-move");
   const auto *ReceivingExpr = Result.Nodes.getNodeAs("receiving-expr");
+  const auto *InvocationParm =
+  Result.Nodes.getNodeAs("invocation-parm");
+
+  if (!ReceivingExpr && HasCheckedMoveSet.contains(CallMove))
+return;
+
+  if (ReceivingExpr)
+HasCheckedMoveSet.insert(CallMove);
+
   const Expr *Arg = CallMove->getArg(0);
   SourceManager  = Result.Context->getSourceManager();
 
@@ -90,20 +101,52 @@
   return;
 
 bool IsVariable = isa(Arg);
+bool IsRValueReferenceArg = false;
 const auto *Var =
 IsVariable ? dyn_cast(Arg)->getDecl() : nullptr;
-auto Diag = diag(FileMoveRange.getBegin(),
- "std::move of the %select{|const }0"
- "%select{expression|variable %4}1 "
- "%select{|of the trivially-copyable type %5 }2"
- "has no effect; remove std::move()"
- "%select{| or make the variable non-const}3")
-<< IsConstArg << IsVariable << IsTriviallyCopyable
-

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

In D108370#3017800 , @aaron.ballman 
wrote:

> LGTM!

Thanks for your review! I don't have commit access, here is my information:
Name: liuke
Email: liuke.ge...@bytedance.com


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

https://reviews.llvm.org/D108370

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

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

In D108370#3017800 , @aaron.ballman 
wrote:

> LGTM!

Thanks for your review!


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

https://reviews.llvm.org/D108370

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

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Hi, Could you please take some time to review this diff again? @whisperity


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

https://reviews.llvm.org/D107450

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

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:484
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))
   return;

aaron.ballman wrote:
> Given that we're touching this code, we might as well make clang-format happy 
> with it (though I can't quite spot what it wants changed, so if it turns out 
> to be a bad idea for some reason, I don't insist).
> Given that we're touching this code, we might as well make clang-format happy 
> with it (though I can't quite spot what it wants changed, so if it turns out 
> to be a bad idea for some reason, I don't insist).

Yes, I have tried to format this code with clang-format but it does not work 
perfectly.


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

https://reviews.llvm.org/D108370

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