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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, rnkovacs, Szelethus.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, baloghadamsoftware.

This patch implements the post important part of the plan proposed in https://reviews.llvm.org/D54556. Under `alpha.cplusplus.Move:Aggressive=true`, the checker retains the original behavior, but when that option is equal to `false` (which is the proposed default behavior), the checker only warns in the following three cases:

1. If the object that's being used after move is of a known STL type (the word "known" here makes no sense; what i'm trying to say is that we'll rant about objects of any STL type, unless we encounter false positives even there and it'll cause us to restrict the check even further). The point here is that according to the C++ Standard, STL objects are known to remain in "valid but unspecified state", which means that the state is going to be internally consistent state which is not known. A lot of operations clearly make no sense (eg., asking for the value of the first character in a string that has just been moved makes little sense), and we know it and we can warn in these cases reliably. If some operations do make sense or, moreover, if some operations return the object into a valid state, we can easily hardcode these operations in the checker because STL has limited amount of stuff in it.
2. If the object is a local or parameter variable. This not only correlates with the true positives that i've seen so far, but also kinda makes sense, though, as usual, i'd love to be proven wrong. When an object is, say, a field in a class, or, even worse, a global, then even if you move from that object, the object is still there. It won't go anywhere; it *will* have to be re-used eventually. Which means that if the object is left in a well-specified state after getting moved, there's no good reason to enforce the developer to reset it manually. However, if the object is a local that is going to be cleaned up soon anyway, it is very unlikely that the desire to re-use its storage is well-justified. It should be totally fine to create another local and use that as a storage for the next object that you need. Hence the idea.
3. If the object is an rvalue reference. The idea is exactly the same as in 2: an xvalue is something that's near the end of its lifetime, so there's no temptation to re-use the storage. Whoever passed us that value is already fine with it being moved away, we have no obligation to fill in the space again.


Repository:
  rC Clang

https://reviews.llvm.org/D54557

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/use-after-move.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D54557.174131.patch
Type: text/x-patch
Size: 13030 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181115/2989fdb8/attachment.bin>