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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
