[PATCH] D124918: [clang-tidy] Add a new check for non-trivial unused variables.

[Luca Versari via Phabricator via cfe-commits]

veluca93 updated this revision to Diff 434026.
veluca93 marked 3 inline comments as done.
veluca93 added a comment.

Respond to comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124918

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp
@@ -0,0 +1,144 @@
+// RUN: %check_clang_tidy %s performance-unused-no-side-effect %t -- \
+// RUN:  -config="{CheckOptions: [{key: \"performance-unused-no-side-effect.TypesByBase\", value: '{\"Marker\":[\"swap\"]}'}]}"
+
+namespace std {
+
+template 
+struct char_traits {};
+
+template 
+struct allocator {};
+
+template <
+class CharT,
+class Traits = std::char_traits,
+class Allocator = std::allocator>
+class basic_string {
+public:
+  basic_string +=(const char *);
+  ~basic_string();
+};
+
+using string = basic_string;
+
+class thread {
+public:
+  ~thread();
+};
+
+template >
+class vector {
+public:
+  void push_back(T);
+};
+
+template 
+class unique_ptr {
+public:
+  unique_ptr(T *);
+};
+
+} // namespace std
+
+struct Marker {
+};
+
+struct SideEffectFree : Marker {
+  SideEffectFree();
+  ~SideEffectFree();
+  void swap(SideEffectFree *);
+};
+
+struct POD {
+  int A;
+};
+
+template 
+struct CustomPair {
+  T t;
+  U u;
+};
+
+void f(const std::string &);
+
+void unusedString() {
+  std::string SomeString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'SomeString' is never read [performance-unused-no-side-effect]
+}
+
+void unusedStringWithMethodCall() {
+  std::string SomeString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'SomeString' is never read [performance-unused-no-side-effect]
+  SomeString += "hi";
+}
+
+void unusedStdThread() {
+  std::thread T;
+}
+
+void usedString() {
+  std::string SomeString;
+  SomeString += "hi";
+  f(SomeString);
+}
+
+void vectorString() {
+  std::vector VecString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: variable 'VecString' is never read [performance-unused-no-side-effect]
+  VecString.push_back(std::string{});
+}
+
+void vectorThread() {
+  std::vector VecThread;
+  VecThread.push_back(std::thread{});
+}
+
+void withMarker() {
+  SideEffectFree M;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: variable 'M' is never read [performance-unused-no-side-effect]
+}
+
+void withMarkerAndExcludedMethod(SideEffectFree *S) {
+  SideEffectFree M;
+  M.swap(S);
+}
+
+void withPOD() {
+  POD P;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'P' is never read [performance-unused-no-side-effect]
+  P.A = 1;
+}
+
+int withPODReturn() {
+  POD P;
+  P.A = 1;
+  return P.A;
+}
+
+void withLoops() {
+  std::vector VecLoops;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: variable 'VecLoops' is never read [performance-unused-no-side-effect]
+  for (int i = 0; i < 15; i++) {
+VecLoops.push_back(1);
+  }
+  int j = 0;
+  while (j < 1) {
+VecLoops.push_back(2);
+j++;
+  }
+  (VecLoops).push_back(3);
+}
+
+void withSmartPointer(int *Arg) {
+  std::unique_ptr Unused(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: variable 'Unused' is never read [performance-unused-no-side-effect]
+
+  std::unique_ptr Used(Arg);
+}
+
+void withCustomPair() {
+  CustomPair> Unused{};
+  // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: variable 'Unused' is never read [performance-unused-no-side-effect]
+
+  CustomPair Used;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
@@ -0,0 +1,199 @@
+.. title:: clang-tidy - performance-unused-no-side-effect
+
+performance-unused-no-side-effect
+=
+
+Finds variables that are not used and do not have user-visible side effects.
+
+This check is similar to the -Wunused-variable and -Wunused-but-set-variable
+compiler warnings; however, it integrates (configurable) knowledge about
+library types and functions to be able to extend these checks to more types of
+variables that do not produce user-visible side effects but i.e. perform
+allocations; not using such variables is almost always a programming error.
+
+A 

[PATCH] D124918: [clang-tidy] Add a new check for non-trivial unused variables.

[Luca Versari via Phabricator via cfe-commits]

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

In D124918#3555719 , @ymandel wrote:

> Luca,
>
> Can you expand the description to give a better intuition as to what this 
> check is finding that the diagnostic does not? The example is good, but it 
> would be helpful if you could phrase the general rule being enforced.  
> Similarly, that would be helpful in the documentation file -- before diving 
> into the formal specification, explain what the intuition. For that matter, 
> why does this merit a separate clang-tidy vs. integeration into the existing 
> code for the diagnostic (or a similar, parallel one)? I'm not advocating one 
> way or another, I'd just like to understand your decision.

I wrote in the doc:

This check is similar to the -Wunused-variable and -Wunused-but-set-variable
compiler warnings; however, it integrates (configurable) knowledge about
library types and functions to be able to extend these checks to more types of
variables that do not produce user-visible side effects but i.e. perform
allocations; not using such variables is almost always a programming error.

The reason for this to be a tidy rather than an extension of the warning is 
twofold:

- I'm not quite confident that this check meets (or can meet) the standard for 
FPR that warnings are held to. It may well be the case, but it's not something 
I checked extensively.
- I don't think compiler warnings can be configured to the extent that would be 
needed for this check to be useful.

> Separately, can you provide some data as to how much of a problem this is in 
> practice? The code here is quite complex and I think that sets a high bar (in 
> terms of benefit to users) for committing it.

I have seen this check trigger about 100k times among ~100M analyzed lines of 
code.

> Thanks!






Comment at: 
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp:90
+if (!Op->isAssignmentOp()) {
+  markSideEffectFree(Op->getRHS());
+}

asoffer wrote:
> Perhaps I'm not understanding precisely what `markSideEffectFree` means, but 
> this doesn't seem right to me:
> 
> ```
> int *data = ...;
> auto getNextEntry = [&] () -> int& { return *++data; };
> int n = (getNextDataEntry() + 17); // We can't mark RHS as side-effect free.
> *data = n;
> ```
`markSideEffectFree` is a bad name - I renamed it to `markMaybeNotObservable`, 
with the idea being that an expression is observable iff any of its ancestors 
in the AST is marked as Observable.



Comment at: 
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h:56
+  llvm::SmallVector SmartPointerTypes;
+  // Map of function name to bitmask of arguments that are not read from.
+  llvm::StringMap> FunctionAllowList;

ymandel wrote:
> can you expand or rephrase? is the idea that these are argument positions 
> that should be ignored for the purposes of this check? If so, what does the 
> name of the field mean?
I renamed the field and expanded the comment.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:116
`bugprone-virtual-near-miss `_, "Yes"
-   `cert-dcl21-cpp `_, "Yes"
+   `cert-dcl21-cpp `_,
`cert-dcl50-cpp `_,

ymandel wrote:
> why the change to this line?
No idea, must have been a merge gone wrong :) Reverted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124918

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

[PATCH] D124918: [clang-tidy] Add a new check for non-trivial unused variables.

[Luca Versari via Phabricator via cfe-commits]

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

@LegalizeAdulthood friendly ping :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124918

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

[PATCH] D124918: [clang-tidy] Add a new check for non-trivial unused variables.

[Luca Versari via Phabricator via cfe-commits]

veluca93 marked an inline comment as done.
veluca93 added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst:172-173
+List of functions that are considered not to read some of their arguments
+unless their return value is read. Arguments are identified by a bitmask, where
+the i-th LSB being set indicates that the i-th argument is unused.
+

LegalizeAdulthood wrote:
> veluca93 wrote:
> > LegalizeAdulthood wrote:
> > > This isn't user-friendly at all.  Why not an array of argument indices 
> > > starting at zero?
> > Done. I'm using an empty array to indicate "everything", which is perhaps a 
> > bit weird.
> I think `[-1]` might be a better sentinel.  What do you do for functions that 
> take zero arguments?
It doesn't matter, right? For the purpose of tracking whether variables are 
used, 0-argument free/static functions are meaningless anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124918

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

[PATCH] D124918: [clang-tidy] Add a new check for non-trivial unused variables.

[Luca Versari via Phabricator via cfe-commits]

veluca93 added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst:93
+   {
+  "std::string": ["swap"],
+  "absl::int128": [],

LegalizeAdulthood wrote:
> Since `std::string` is just a type alias, shouldn't we be considering 
> `basic_string`?
> What about `wstring`?
Looks like std::string is indeed unnecessary.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst:172-173
+List of functions that are considered not to read some of their arguments
+unless their return value is read. Arguments are identified by a bitmask, where
+the i-th LSB being set indicates that the i-th argument is unused.
+

LegalizeAdulthood wrote:
> This isn't user-friendly at all.  Why not an array of argument indices 
> starting at zero?
Done. I'm using an empty array to indicate "everything", which is perhaps a bit 
weird.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124918

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

[PATCH] D124918: [clang-tidy] Add a new check for non-trivial unused variables.

[Luca Versari via Phabricator via cfe-commits]

veluca93 updated this revision to Diff 429745.
veluca93 marked 6 inline comments as done.
veluca93 added a comment.

Switch from bitmasks to arrays. Add more tests & update doc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124918

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp
@@ -0,0 +1,144 @@
+// RUN: %check_clang_tidy %s performance-unused-no-side-effect %t -- \
+// RUN:  -config="{CheckOptions: [{key: \"performance-unused-no-side-effect.TypesByBase\", value: '{\"Marker\":[\"swap\"]}'}]}"
+
+namespace std {
+
+template 
+struct char_traits {};
+
+template 
+struct allocator {};
+
+template <
+class CharT,
+class Traits = std::char_traits,
+class Allocator = std::allocator>
+class basic_string {
+public:
+  basic_string +=(const char *);
+  ~basic_string();
+};
+
+using string = basic_string;
+
+class thread {
+public:
+  ~thread();
+};
+
+template >
+class vector {
+public:
+  void push_back(T);
+};
+
+template 
+class unique_ptr {
+public:
+  unique_ptr(T *);
+};
+
+} // namespace std
+
+struct Marker {
+};
+
+struct SideEffectFree : Marker {
+  SideEffectFree();
+  ~SideEffectFree();
+  void swap(SideEffectFree *);
+};
+
+struct POD {
+  int A;
+};
+
+template 
+struct CustomPair {
+  T t;
+  U u;
+};
+
+void f(const std::string &);
+
+void unusedString() {
+  std::string SomeString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'SomeString' is never read [performance-unused-no-side-effect]
+}
+
+void unusedStringWithMethodCall() {
+  std::string SomeString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'SomeString' is never read [performance-unused-no-side-effect]
+  SomeString += "hi";
+}
+
+void unusedStdThread() {
+  std::thread T;
+}
+
+void usedString() {
+  std::string SomeString;
+  SomeString += "hi";
+  f(SomeString);
+}
+
+void vectorString() {
+  std::vector VecString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: variable 'VecString' is never read [performance-unused-no-side-effect]
+  VecString.push_back(std::string{});
+}
+
+void vectorThread() {
+  std::vector VecThread;
+  VecThread.push_back(std::thread{});
+}
+
+void withMarker() {
+  SideEffectFree M;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: variable 'M' is never read [performance-unused-no-side-effect]
+}
+
+void withMarkerAndExcludedMethod(SideEffectFree *S) {
+  SideEffectFree M;
+  M.swap(S);
+}
+
+void withPOD() {
+  POD P;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'P' is never read [performance-unused-no-side-effect]
+  P.A = 1;
+}
+
+int withPODReturn() {
+  POD P;
+  P.A = 1;
+  return P.A;
+}
+
+void withLoops() {
+  std::vector VecLoops;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: variable 'VecLoops' is never read [performance-unused-no-side-effect]
+  for (int i = 0; i < 15; i++) {
+VecLoops.push_back(1);
+  }
+  int j = 0;
+  while (j < 1) {
+VecLoops.push_back(2);
+j++;
+  }
+  (VecLoops).push_back(3);
+}
+
+void withSmartPointer(int *Arg) {
+  std::unique_ptr Unused(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: variable 'Unused' is never read [performance-unused-no-side-effect]
+
+  std::unique_ptr Used(Arg);
+}
+
+void withCustomPair() {
+  CustomPair> Unused{};
+  // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: variable 'Unused' is never read [performance-unused-no-side-effect]
+
+  CustomPair Used;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
@@ -0,0 +1,193 @@
+.. title:: clang-tidy - performance-unused-no-side-effect
+
+performance-unused-no-side-effect
+=
+
+Finds variables that are not used and do not have user-visible side effects.
+
+A variable is defined to be side-effect-free and unused if:
+
+- It has a side-effect-free type (defined below).
+- It is not passed to a function that reads its content and whose result is used.
+
+A type is side-effect-free if it is any of the following:
+
+- A type with a base class specified in `TypesByBase`.
+- A type listed in `RawTypes`.
+- 

[PATCH] D124918: [clang-tidy] Add a new check for non-trivial unused variables.

[Luca Versari via Phabricator via cfe-commits]

veluca93 marked an inline comment as done.
veluca93 added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp:135
+// or class, and that there are no side effects on `this`.
+if (auto *Callee = dyn_cast(Call->getCallee())) {
+  markSideEffectFree(Callee);

Eugene.Zelenko wrote:
> `const auto *`. Same in other similar places.
Somehow I missed a couple of those... should be fixed now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124918

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

[PATCH] D124918: [clang-tidy] Add a new check for non-trivial unused variables.

[Luca Versari via Phabricator via cfe-commits]

veluca93 updated this revision to Diff 427013.
veluca93 added a comment.

Missing `const` from auto *


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124918

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp
@@ -0,0 +1,118 @@
+// RUN: %check_clang_tidy %s performance-unused-no-side-effect %t -- \
+// RUN:  -config="{CheckOptions: [{key: \"performance-unused-no-side-effect.TypesByBase\", value: '{\"Marker\":[\"swap\"]}'}]}"
+
+namespace std {
+
+template 
+struct char_traits {};
+
+template 
+struct allocator {};
+
+template <
+class CharT,
+class Traits = std::char_traits,
+class Allocator = std::allocator>
+class basic_string {
+public:
+  basic_string +=(const char *);
+  ~basic_string();
+};
+
+using string = basic_string;
+
+class thread {
+public:
+  ~thread();
+};
+
+template >
+class vector {
+public:
+  void push_back(T);
+};
+
+} // namespace std
+
+struct Marker {
+};
+
+struct SideEffectFree : Marker {
+  SideEffectFree();
+  ~SideEffectFree();
+  void swap(SideEffectFree *);
+};
+
+struct POD {
+  int A;
+};
+
+void f(const std::string &);
+
+void unusedString() {
+  std::string SomeString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'SomeString' is never read [performance-unused-no-side-effect]
+}
+
+void unusedStringWithMethodCall() {
+  std::string SomeString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'SomeString' is never read [performance-unused-no-side-effect]
+  SomeString += "hi";
+}
+
+void unusedStdThread() {
+  std::thread T;
+}
+
+void usedString() {
+  std::string SomeString;
+  SomeString += "hi";
+  f(SomeString);
+}
+
+void vectorString() {
+  std::vector VecString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: variable 'VecString' is never read [performance-unused-no-side-effect]
+  VecString.push_back(std::string{});
+}
+
+void vectorThread() {
+  std::vector VecThread;
+  VecThread.push_back(std::thread{});
+}
+
+void withMarker() {
+  SideEffectFree M;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: variable 'M' is never read [performance-unused-no-side-effect]
+}
+
+void withMarkerAndExcludedMethod(SideEffectFree *S) {
+  SideEffectFree M;
+  M.swap(S);
+}
+
+void withPOD() {
+  POD P;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'P' is never read [performance-unused-no-side-effect]
+  P.A = 1;
+}
+
+int withPODReturn() {
+  POD P;
+  P.A = 1;
+  return P.A;
+}
+
+int main() {
+  std::vector VecLoops;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: variable 'VecLoops' is never read [performance-unused-no-side-effect]
+  for (int i = 0; i < 15; i++) {
+VecLoops.push_back(1);
+  }
+  int j = 0;
+  while (j < 1) {
+VecLoops.push_back(2);
+j++;
+  }
+  (VecLoops).push_back(3);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
@@ -0,0 +1,193 @@
+.. title:: clang-tidy - performance-unused-no-side-effect
+
+performance-unused-no-side-effect
+=
+
+Finds variables that are not used and do not have user-visible side effects.
+
+A variable is defined to be side-effect-free and unused if:
+
+- It has a side-effect-free type (defined below).
+- It is not passed to a function that reads its content and whose result is used.
+
+A type is side-effect-free if it is any of the following:
+
+- A type with a base class specified in `TypesByBase`.
+- A type listed in `RawTypes`.
+- A template type listed in `TemplateTypes` whose template arguments are
+  themselves side-effect-free types.
+- A smart pointer (listed in `SmartPointerTypes`) whose pointee is of
+  side-effect-free type.
+- A primitive type.
+- A pointer.
+- A struct/class with no user-defined destructor and members of trivial type.
+
+A function is considered to "not read an argument unless the return value is
+used" in the following cases:
+
+- It is a method of the explicitly listed types and the argument is ``this``,
+  except:
+
+  - The exceptions associated with the type (eg. ``std::vector::swap``).
+  - A constructor 

[PATCH] D124918: [clang-tidy] Add a new check for non-trivial unused variables.

[Luca Versari via Phabricator via cfe-commits]

veluca93 updated this revision to Diff 427007.
veluca93 marked 7 inline comments as done.
veluca93 added a comment.

Address first round of review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124918

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp
@@ -0,0 +1,118 @@
+// RUN: %check_clang_tidy %s performance-unused-no-side-effect %t -- \
+// RUN:  -config="{CheckOptions: [{key: \"performance-unused-no-side-effect.TypesByBase\", value: '{\"Marker\":[\"swap\"]}'}]}"
+
+namespace std {
+
+template 
+struct char_traits {};
+
+template 
+struct allocator {};
+
+template <
+class CharT,
+class Traits = std::char_traits,
+class Allocator = std::allocator>
+class basic_string {
+public:
+  basic_string +=(const char *);
+  ~basic_string();
+};
+
+using string = basic_string;
+
+class thread {
+public:
+  ~thread();
+};
+
+template >
+class vector {
+public:
+  void push_back(T);
+};
+
+} // namespace std
+
+struct Marker {
+};
+
+struct SideEffectFree : Marker {
+  SideEffectFree();
+  ~SideEffectFree();
+  void swap(SideEffectFree *);
+};
+
+struct POD {
+  int A;
+};
+
+void f(const std::string &);
+
+void unusedString() {
+  std::string SomeString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'SomeString' is never read [performance-unused-no-side-effect]
+}
+
+void unusedStringWithMethodCall() {
+  std::string SomeString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'SomeString' is never read [performance-unused-no-side-effect]
+  SomeString += "hi";
+}
+
+void unusedStdThread() {
+  std::thread T;
+}
+
+void usedString() {
+  std::string SomeString;
+  SomeString += "hi";
+  f(SomeString);
+}
+
+void vectorString() {
+  std::vector VecString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: variable 'VecString' is never read [performance-unused-no-side-effect]
+  VecString.push_back(std::string{});
+}
+
+void vectorThread() {
+  std::vector VecThread;
+  VecThread.push_back(std::thread{});
+}
+
+void withMarker() {
+  SideEffectFree M;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: variable 'M' is never read [performance-unused-no-side-effect]
+}
+
+void withMarkerAndExcludedMethod(SideEffectFree *S) {
+  SideEffectFree M;
+  M.swap(S);
+}
+
+void withPOD() {
+  POD P;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'P' is never read [performance-unused-no-side-effect]
+  P.A = 1;
+}
+
+int withPODReturn() {
+  POD P;
+  P.A = 1;
+  return P.A;
+}
+
+int main() {
+  std::vector VecLoops;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: variable 'VecLoops' is never read [performance-unused-no-side-effect]
+  for (int i = 0; i < 15; i++) {
+VecLoops.push_back(1);
+  }
+  int j = 0;
+  while (j < 1) {
+VecLoops.push_back(2);
+j++;
+  }
+  (VecLoops).push_back(3);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
@@ -0,0 +1,193 @@
+.. title:: clang-tidy - performance-unused-no-side-effect
+
+performance-unused-no-side-effect
+=
+
+Finds variables that are not used and do not have user-visible side effects.
+
+A variable is defined to be side-effect-free and unused if:
+
+- It has a side-effect-free type (defined below).
+- It is not passed to a function that reads its content and whose result is used.
+
+A type is side-effect-free if it is any of the following:
+
+- A type with a base class specified in `TypesByBase`.
+- A type listed in `RawTypes`.
+- A template type listed in `TemplateTypes` whose template arguments are
+  themselves side-effect-free types.
+- A smart pointer (listed in `SmartPointerTypes`) whose pointee is of
+  side-effect-free type.
+- A primitive type.
+- A pointer.
+- A struct/class with no user-defined destructor and members of trivial type.
+
+A function is considered to "not read an argument unless the return value is
+used" in the following cases:
+
+- It is a method of the explicitly listed types and the argument is ``this``,
+  except:
+
+  - The exceptions associated with the 

[PATCH] D124918: Add a new clang-tidy for non-trivial unused variables.

[Luca Versari via Phabricator via cfe-commits]

veluca93 created this revision.
Herald added subscribers: carlosgalvezp, mgorny.
Herald added a project: All.
veluca93 requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

This check is meant to detect case that -Wunused-variable does not,
by integrating some extra knowledge of types. For example, a
std::vector that only ever gets push_back()-ed is similar to a
written-but-never-read variable.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124918

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp
@@ -0,0 +1,118 @@
+// RUN: %check_clang_tidy %s performance-unused-no-side-effect %t -- \
+// RUN:  -config="{CheckOptions: [{key: \"performance-unused-no-side-effect.TypesByBase\", value: '{\"Marker\":[\"swap\"]}'}]}"
+
+namespace std {
+
+template 
+struct char_traits {};
+
+template 
+struct allocator {};
+
+template <
+class CharT,
+class Traits = std::char_traits,
+class Allocator = std::allocator>
+class basic_string {
+public:
+  basic_string +=(const char *);
+  ~basic_string();
+};
+
+using string = basic_string;
+
+class thread {
+public:
+  ~thread();
+};
+
+template >
+class vector {
+public:
+  void push_back(T);
+};
+
+} // namespace std
+
+struct Marker {
+};
+
+struct SideEffectFree : Marker {
+  SideEffectFree();
+  ~SideEffectFree();
+  void swap(SideEffectFree *);
+};
+
+struct POD {
+  int A;
+};
+
+void f(const std::string &);
+
+void unusedString() {
+  std::string SomeString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'SomeString' is never read [performance-unused-no-side-effect]
+}
+
+void unusedStringWithMethodCall() {
+  std::string SomeString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'SomeString' is never read [performance-unused-no-side-effect]
+  SomeString += "hi";
+}
+
+void unusedStdThread() {
+  std::thread T;
+}
+
+void usedString() {
+  std::string SomeString;
+  SomeString += "hi";
+  f(SomeString);
+}
+
+void vectorString() {
+  std::vector VecString;
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: variable 'VecString' is never read [performance-unused-no-side-effect]
+  VecString.push_back(std::string{});
+}
+
+void vectorThread() {
+  std::vector VecThread;
+  VecThread.push_back(std::thread{});
+}
+
+void withMarker() {
+  SideEffectFree M;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: variable 'M' is never read [performance-unused-no-side-effect]
+}
+
+void withMarkerAndExcludedMethod(SideEffectFree *S) {
+  SideEffectFree M;
+  M.swap(S);
+}
+
+void withPOD() {
+  POD P;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'P' is never read [performance-unused-no-side-effect]
+  P.A = 1;
+}
+
+int withPODReturn() {
+  POD P;
+  P.A = 1;
+  return P.A;
+}
+
+int main() {
+  std::vector VecLoops;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: variable 'VecLoops' is never read [performance-unused-no-side-effect]
+  for (int i = 0; i < 15; i++) {
+VecLoops.push_back(1);
+  }
+  int j = 0;
+  while (j < 1) {
+VecLoops.push_back(2);
+j++;
+  }
+  (VecLoops).push_back(3);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
@@ -0,0 +1,192 @@
+.. title:: clang-tidy - performance-unused-no-side-effect
+
+performance-unused-no-side-effect
+=
+
+Finds variables that are not used and do not have user-visible side effects.
+
+A variable is defined to be side-effect-free and unused if
+
+- It has a side-effect-free type (defined below)
+- It is not passed to a function that reads its content and whose result is used
+
+A type is side-effect-free if it is:
+
+- A type with a base class specified in TypesByBase
+- A type listed in RawTypes
+- A template type listed in TemplateTypes whose template arguments are
+  themselves side-effect-free types
+- A smart pointer (listed in SmartPointerTypes) whose pointee is of side-effect-free type.
+- A primitive type.
+- A pointer.
+- A struct/class with no user-defined destructor and members of trivial type.
+
+A function is considered to "not read an