[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 reviewer: rtrieu.
george.burgess.iv added a comment.

Just a few more nits and LGTM. We probably want the thoughts of someone with ownership in warnings to be sure. +rtrieu might be good?



================
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) {
----------------
mbenfield wrote:
> george.burgess.iv wrote:
> > nit: Should this be a `const Stmt*`? I don't think we should be mutating the `Body`
> Unfortunately the `RecursiveASTVisitor`'s non-overridden member functions don't take `const`.
Yeah, I was thinking of StmtVisitor's interface -- my mistake


================
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;
----------------
mbenfield wrote:
> mbenfield wrote:
> > george.burgess.iv wrote:
> > > If we want to optimize for when this warning is disabled, would it be better to hoist this to the start of DiagnoseUnusedButSetParameters?
> > Isn't it essentially at the beginning of the function as-is? If the warning is disabled for all parameters, it'll return false right away for all of them. However, I will add an extra check to end as soon as possible once no candidates are found. Let me know if it isn't ideal.
> I didn't modify this. I'm not sure there would be any advantage to checking if warnings are disabled for every parameter before just doing all the checks for all of them. Please push back if you think otherwise. 
Ah, I was misreading a bit; didn't realize that we were using the `SourceLoc` of each individual parameter to feed into `getDiagnosticLevel`. Yeah, this is fine as-is.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13752
+      // This is not an assignment to one of our NamedDecls.
+      TraverseStmt(LHS);
+    }
----------------
since we try to exit early, should this be

```
if (!TraverseStmt(LHS))
  return false;
```

(and similar below)?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13760
+    // If we remove all Decls, no need to keep searching.
+    return (!S.erase(DRE->getFoundDecl()) || S.size());
+  }
----------------
nit: LLVM doesn't like superfluous parens; please write this as `return !S.erase(DRE->getFoundDecl()) || S.size();`


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13825
+  Decl *D = Decl::castFromDeclContext((*Parameters.begin())->getDeclContext());
+  if (D)
+    DiagnoseUnusedButSetDecls(this, D, Candidates,
----------------
nit: LLVM generally tries to write

```
auto *Foo = bar();
if (Foo)
  use(Foo);
```

as

```
if (auto *Foo = bar())
  use(Foo)
```

in part because the latter makes a bit better use of scopes


================
Comment at: clang/test/Sema/vector-gcc-compat.c:1
 // RUN: %clang_cc1 %s -verify -fsyntax-only -Weverything -triple x86_64-apple-darwin10
 
----------------
nit: is it possible to add -Wno-unused-but-set-parameter & -Wno-unused-but-set-variable as a part of the RUN line?

if it becomes too long, you can use \ to wrap it:

```
// RUN: foo bar \
// RUN:     baz
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100581