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

[Michael Benfield via Phabricator via cfe-commits]

mbenfield marked 4 inline comments as done.
mbenfield added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13751
+    bool TraverseBinaryOperator(BinaryOperator *BO) {
+      auto *LHS = BO->getLHS();
+      auto *DRE = dyn_cast<DeclRefExpr>(LHS);
----------------
george.burgess.iv wrote:
> nit: `const auto *` is preferred when possible
Not possible here. TraverseStmt doesn't take a const parameter. 


================
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;
----------------
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.


================
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:
> 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. 


================
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()) ==
----------------
george.burgess.iv wrote:
> Same "why not put this at the beginning of `DiagnoseUnusedButSetVariables`?" comment
Ditto. 


================
Comment at: clang/lib/Sema/SemaStmt.cpp:437
+  CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R);
+  DiagnoseUnusedButSetVariables(CS);
+  return CS;
----------------
george.burgess.iv wrote:
> 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?
I'll think about this. It might be tricky. 


================
Comment at: clang/lib/Sema/SemaStmt.cpp:437
+  CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R);
+  DiagnoseUnusedButSetVariables(CS);
+  return CS;
----------------
mbenfield wrote:
> george.burgess.iv wrote:
> > 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?
> I'll think about this. It might be tricky. 
I'll implement this. It will be a fairly different approach though. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100581