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<NoSideEffectsInfo> TypesByBase;
+ llvm::SmallVector<NoSideEffectsInfo> RawTypes;
+ llvm::SmallVector<NoSideEffectsInfo> TemplateTypes;
+ llvm::SmallVector<NoSideEffectsInfo> 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<NoSideEffectsInfo> SmartPointerTypes;
+ // Map of function name to bitmask of arguments that are not read from.
+ llvm::StringMap<llvm::SmallSet<size_t, 4>> 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<llvm::SmallSet<size_t, 4>> 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 <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>`_,
----------------
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
