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

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added inline comments.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:437
+  CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R);
+  DiagnoseUnusedButSetVariables(CS);
+  return CS;
----------------
mbenfield wrote:
> mbenfield wrote:
> > 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. 
> I have this implemented. I am going to run a performance comparison on the two approaches before making a new revision here. 
An informal comparison reveals no significant performance difference between these two approaches. I think it's a tradeoff: if you walk each block and/or each function separately, you do have to do more walks, but you're likely to be able to terminate your walk early (because you already ran into a use of each variable/parameter).

OTOH, if you walk everything in one go, it is only one walk, but you definitely have to walk the whole thing (because you might run into another CompoundStmt with more declarations). 

I find the current approach conceptually simpler and so would prefer to keep it as-is. If you disagree let me know. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100581