[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

I think we should either remove the non-default functionality (which wouldn't be ideal), or emphasise somewhere (open projects?) that there is still work to be done, but leaving it to be forgotten and essentially making it an extra maintenance work would be, in my optinion, the worst case scenario. `Aggressive` isn't `Pedantic` because it actually emits warnings on correct code, and it's not a simple matter of too many reports being emitted, let's also document that this is an experimental feature, not a power-user-only thing.

Whenever I compile `Rtags`, I get a bunch of move related warnings, I'm curious how this patch behaves on that project. I'll take a look.



================
Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:92-95
+  bool IsAggressive = false;
+
+public:
+  void setAggressiveness(bool Aggressive) { IsAggressive = Aggressive; }
----------------
How about a simple public data member? Since all callbacks are const, it wouldn't be modified after registration anyways.


================
Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:134
+static const MemRegion *unwrapRValueReferenceIndirection(const MemRegion *MR) {
+  if (const auto *SR = dyn_cast_or_null<SymbolicRegion>(MR)) {
+    SymbolRef Sym = SR->getSymbol();
----------------
You use `dyn_cast_or_null`, which to me implies that the returned value may be `nullptr`, but you never check it on the call sites.


================
Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:273-282
+  // In non-aggressive mode, only warn on use-after-move of local variables (or
+  // local rvalue references) and of STL objects. The former is possible because
+  // local variables (or local rvalue references) are not tempting their user to
+  // re-use the storage. The latter is possible because STL objects are known
+  // to end up in a valid but unspecified state after the move and their
+  // state-reset methods are also known, which allows us to predict
+  // precisely when use-after-move is invalid.
----------------
I've seen checker option descriptions in registry functions, in-class where they are declared, and on top of the file, I think any of them would be more ideal. Since my checker option related efforts are still far in the distance (which will put all of them in `Checkers.td`), let's move (or at least copy) this somewhere else.


================
Comment at: test/Analysis/use-after-move.cpp:47-52
+template <typename T>
+class vector {
+public:
+  vector();
+  void push_back(const T &t);
+};
----------------
Any particular reason for not using `cxx-system-header-simulator.h`?


Repository:
  rC Clang

https://reviews.llvm.org/D54557