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