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

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


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

[Andy Soffer via Phabricator via cfe-commits]

asoffer added inline comments.



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

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;
```


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.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

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.

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.

Thanks!




Comment at: 
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h:52-55
+  llvm::SmallVector TypesByBase;
+  llvm::SmallVector RawTypes;
+  llvm::SmallVector TemplateTypes;
+  llvm::SmallVector SmartPointerTypes;

Please add comments explaining the roles of these fields (either individually 
or collectively)



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;

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?



Comment at: 
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h:58-59
+  llvm::StringMap> FunctionAllowList;
+  // TODO(veluca): The FixIts don't really fix everything and can break code.
+  static constexpr bool EmitFixits = false;
+};

Might this be better in the `.cpp` file?



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 `_,

why the change to this line?


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.

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood 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.
+

veluca93 wrote:
> 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.
OK, yes, I was thinking member functions not free/static functions.


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.

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood 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.
+

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?


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.

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst:25-26
+
+A function is considered to "not read an argument unless the return value is
+used" in the following cases:
+

The quotes here don't feel like they're needed.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst:35
+
+- A function listed in FunctionAllowList, if the argument is in index ``i`` and
+  bit ``i`` is set in the corresponding allow list entry.

Single backquotes around `FunctionAllowList`



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

Since `std::string` is just a type alias, shouldn't we be considering 
`basic_string`?
What about `wstring`?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst:103
+  "absl::string_view": ["swap", "copy"],
+  "std::string_view": ["swap", "copy"]
+   }

`string_view` is also just a type alias, there is `wstring_view`, etc.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst:115-152
+  {
+"std::vector": ["swap"],
+"std::pair": [],
+"std::tuple": [],
+"std::optional": ["swap"],
+"std::variant": ["swap"],
+"std::list": ["swap"],

It would be easier to scan this list if it were sorted alphabetically.  I see 
`basic_string` listed here, but not `basic_string_view`.  Is that intentional?



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.
+

This isn't user-friendly at all.  Why not an array of argument indices starting 
at zero?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp:118
+  (VecLoops).push_back(3);
+}

Should there be some test cases for user-defined template types with no side 
effects?

Also some test cases for the smart pointer cases?


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/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.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko 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);

`const auto *`. Same in other similar places.


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 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: [clang-tidy] Add a new check for non-trivial unused variables.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp:11
+
+#include 
+#include 

C++ headers should be after application's ones.



Comment at: 
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp:116
+CalleesForStmts[CurrentExprOrDeclStmt].insert(Callee);
+auto *CXXMethod = dyn_cast(Callee);
+if (!CXXMethod || CXXMethod->isStatic()) {

`const auto *`. Same in other places.



Comment at: 
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h:12
+
+#include 
+

C++ headers should be after application's ones.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst:8
+
+A variable is defined to be side-effect-free and unused if
+

Missing colon?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst:10
+
+- 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

Missing period. Same in other places.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst:15
+
+- A type with a base class specified in TypesByBase
+- A type listed in RawTypes

Please highlight `TypesByBase` with single back-ticks. Same for other elements 
of JSON.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst:27
+
+- It is a method of the explicitly listed types and the argument is `this`,
+  except:

Please use double back-ticks for language constructs like `this`.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst:30
+
+  - The exceptions associated with the type (eg. std::vector::swap).
+  - A constructor of a smart pointer type that takes a non-newly-allocated

Please highlight `std::vector::swap` with double back-ticks.


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