[PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'

[Martin Böhme via cfe-commits]

mboehme marked 9 inline comments as done.

================
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:493
@@ +492,3 @@
+    if (!S)
+      continue;
+
----------------
For some reason, I thought I had read that they weren't compatible with CFG / CFGBlock -- but obviously I must have been imagining things. ;)

Also changed the other occurrences.

================
Comment at: docs/clang-tidy/checks/misc-use-after-move.rst:16
@@ +15,3 @@
+The last line will trigger a warning that ``str`` is used after it has been
+moved.
+
----------------
There doesn't seem to be a clear preference here (see alexfh's comments on similar cases), so I'll leave this open until it's resolved one way or the other.

================
Comment at: docs/clang-tidy/checks/misc-use-after-move.rst:182
@@ +181,3 @@
+
+    struct S {
+      std::string str;
----------------
Sorry -- forgot to check that the documentation compiles (it does now).

================
Comment at: test/clang-tidy/misc-use-after-move.cpp:159
@@ +158,3 @@
+    ptr.get();
+  }
+  // This is also true if the std::unique_ptr<> or std::shared_ptr<> is wrapped
----------------
No, it doesn't. At the moment, the check intentionally disregards all uses of unique_ptr and shared_ptr (see also the documentation).

I agree that it definitely makes sense to check for scenarios like the one you mention. They're a bit of a different beast though because unique_ptr and shared_ptr have a well-defined state after they've been moved from. This means they would require some special logic -- we'd want to disallow ptr->Foo() after a std::move, but not ptr.get(). For this reason, I've left them out of this initial version.

Also, I'm not sure whether this check is the best place for these unique_ptr and shared_ptr checks to live. Because the after-move state of unique_ptr and shared_ptr is well defined, the "use, then dereference" case is really just a subset of what could be a more general "dereference null pointer" check.

================
Comment at: test/clang-tidy/misc-use-after-move.cpp:280
@@ +279,3 @@
+    A a;
+    std::move(a);
+    auto lambda = [&]() { a.foo(); };
----------------
> can you add tests with reference capture?

Done.

> also what about: [snip]

The check won't warn about this. More generally, it doesn't do any kind of inter-procedural analysis.

Inter-procedural analysis would certainly help in a number of ways. For example, it could be used to answer the following:

- If a function takes a non-const reference to an object, does it reinitialize that object? (Currently, we optimistically assume that it always does.)
- If a function (that isn't a move constructor or move assignment operator) takes an rvalue reference to an object, does it actually move from that object, and does it do so unconditionally?

However, this would take significant additional implementation effort and would also run more slowly.


https://reviews.llvm.org/D23353