[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this! I think this warning looks valuable.

Most of my comments are various forms of style nits. :)



================
Comment at: clang/lib/Sema/SemaDecl.cpp:13738
 
+// values in Map should be true. traverses Body; if any key is used in any way
+// other than assigning to it, sets the corresponding value to false.
----------------
nit: Please use `///` for documenting functions and methods

secondary nit: it looks like the prevailing comment style here is `// Capitalized sentences ending with a period.` Please try to keep with that style (here and below)


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13740
+// other than assigning to it, sets the corresponding value to false.
+static void AreAllUsesSets(Stmt *Body,
+                           llvm::SmallDenseMap<NamedDecl *, bool> *Map) {
----------------
nit: Should this be a `const Stmt*`? I don't think we should be mutating the `Body`


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13744
+  struct AllUsesAreSetsVisitor : RecursiveASTVisitor<AllUsesAreSetsVisitor> {
+    llvm::SmallDenseMap<NamedDecl *, bool> *M;
+    unsigned FalseCount = 0;
----------------
nit: For visitors that're meant primarily to update an external data structure and be discarded, I think the preferred style is that they take and hold references rather than pointers

Also, now that I'm looking at this again, could `M` be a `SmallPtrSet<NamedDecl *>`? The idea would be "this is the set of things we've not seen outside of an assignment so far." 


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13751
+    bool TraverseBinaryOperator(BinaryOperator *BO) {
+      auto *LHS = BO->getLHS();
+      auto *DRE = dyn_cast<DeclRefExpr>(LHS);
----------------
nit: `const auto *` is preferred when possible


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13763
+      auto iter = M->find(DRE->getFoundDecl());
+      if (iter != M->end() && iter->second) {
+        // we've found a use of this Decl that's not the LHS of an assignment
----------------
nit: LLVM prefers early exits over reuniting control flow, so this should probably be:

```
if (iter == M->end() || !iter->second) {
  return true;
}

iter->second = false;
++FalseCount;
return FalseCount != M->size();
```


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13812
+
+  auto IsCandidate = [CPlusPlus, &Diags = Diags](ParmVarDecl *P) {
+    // check for Ignored here, because if we have no candidates we can avoid
----------------
nit: For lambdas which don't escape, I think the style preference is simply `[&]`


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13813-13818
+    // check for Ignored here, because if we have no candidates we can avoid
+    // walking the AST
+    if (Diags.getDiagnosticLevel(diag::warn_unused_but_set_parameter,
+                                 P->getLocation()) ==
+        DiagnosticsEngine::Ignored)
+      return false;
----------------
If we want to optimize for when this warning is disabled, would it be better to hoist this to the start of DiagnoseUnusedButSetParameters?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13850-13853
+    // check for Ignored here, because if we have no candidates we can avoid
+    // walking the AST
+    if (Diags.getDiagnosticLevel(diag::warn_unused_but_set_variable,
+                                 VD->getLocation()) ==
----------------
Same "why not put this at the beginning of `DiagnoseUnusedButSetVariables`?" comment


================
Comment at: clang/lib/Sema/SemaStmt.cpp:437
+  CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R);
+  DiagnoseUnusedButSetVariables(CS);
+  return CS;
----------------
It seems like these two warnings are doing analyses with identical requirements (e.g., the function's body must be present), but are taking two different sets of variables into account while doing so.

Is there a way we can rephrase this so we only need to walk the AST checking once for all of this?


================
Comment at: clang/test/Sema/warn-unused-but-set-variables-cpp.cpp:14
+
+  // no warning for structs in C++
+  struct S s;
----------------
Can you please expand on why not?

I can see a case for structs with nontrivial dtors or custom `operator=` methods. That said I don't understand why we'd avoid this for trivial structs like `S`.

(edit: now that I'm seeing this is for GCC compatibility, please call that out as a part of this comment. Maybe we want to do better than GCC? That can easily be done in a follow-up patch/commit though, so no strong opinion from me)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581