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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D100581#2706554 <https://reviews.llvm.org/D100581#2706554>, @xbolva00 wrote:

> In D100581#2706467 <https://reviews.llvm.org/D100581#2706467>, @dblaikie wrote:
>
>> FWIW, I'd love it if we could do a full dead-store warning, which would be a superset of this. I think we have enough infrastructure in the analysis based warnings (I think the sufficiency of the infrastructure is demonstrated by the "may be used uninitialized" warnings). Such a warning would subsume these narrower "set but not used" type of warnings (though would require the analysis warning infrastructure).
>
>
>
> - Compile time cost could be a problem.

Given that we managed to enable -Wsometimes-uninitialized, which is a CFG-sensitive warning under -Wall:

  $ clang++-tot maybe-uninit.cpp -Wall -c
  maybe-uninit.cpp:4:7: warning: variable 'i' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    if (b)
        ^
  maybe-uninit.cpp:6:6: note: uninitialized use occurs here
    f1(i);
       ^
  maybe-uninit.cpp:4:3: note: remove the 'if' if its condition is always true
    if (b)
    ^~~~~~
  maybe-uninit.cpp:3:8: note: initialize the variable 'i' to silence this warning
    int i;
         ^
          = 0
  1 warning generated.

Then I think similar infrastructure could be used to implement a dead-store warning too.

> - Do we need stronger dead store warning? Clang analyzer checks for dead stores, no?

Catching something at compile time's a fair bit of increased value over using the static analyzer (at least at Google we don't use the static analyzer, for instance)

Oh, right - https://bugs.llvm.org/show_bug.cgi?id=24506 documents one of the cases that came up that I think a dead store warning would be rather helpful at catching. (though I think we probably now catch the int case -Wzero-as-null-pointer-constant - though that's not on by default, for instance because it's too pervasive, but a dead store warning would catch the buggy case without require a stylistic change to a whole codebase - and also a dead store warning could catch the case with, say a T** parameter where you mean to set the caller's value to nullptr, but instead set the local copy to nullptr) - in these simple examples unused-but-set catches them, but it's not uncommon to test a parameter for non-null before writing, for instance you might mean "if (x) *x = nullptr" but write "if (x) x = nullptr" - dead store would catch that but unused-but-set would not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100581