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