[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 <https://reviews.llvm.org/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<NoSideEffectsInfo> SmartPointerTypes;
+  // Map of function name to bitmask of arguments that are not read from.
+  llvm::StringMap<llvm::SmallSet<size_t, 4>> 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 <bugprone-virtual-near-miss.html>`_, "Yes"
-   `cert-dcl21-cpp <cert-dcl21-cpp.html>`_, "Yes"
+   `cert-dcl21-cpp <cert-dcl21-cpp.html>`_,
    `cert-dcl50-cpp <cert-dcl50-cpp.html>`_,
----------------
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