[PATCH] D10425: Warn, if parameter is used in ctor body after being used for move-construct

[Ismail Pazarbasi]

ismailp added a comment.

In http://reviews.llvm.org/D10425#194696, @dblaikie wrote:

> A few thoughts - but probably having some review by rtreiu would be good. He's more involved in the diagnostic development than I am.
>
> Have you run this over any substantial existing codebase to see what the stats are like (true/false positives, etc)?


No, I haven't, but I heard Google has a large C++11 codebase :) I saw this bug in clang's source code (I didn't submit patch for it yet, can do it in a few days).

> The other issue is that, of course, we'd like a much more general version of this warning but it's unclear how much work that'll be and what the right tradeoff is for compile time. Some common analysis might be shared between a bunch of such warnings - we already have "sometimes-uninitialized" which is a CFG diagnostic that could be useful (treat moved-from as the uninitialized state and do the same analysis).


I'll take a look at that and see whether this analysis can be merged into sometimes-uninitialized.


================
Comment at: lib/Sema/SemaDecl.cpp:10694
@@ +10693,3 @@
+    FieldInitMovedFromParamVisitor V(FD->getASTContext(), FD->getName());
+    V.Visit(E);
+    if (ParmVarDecl *P = V.getParam())
----------------
dblaikie wrote:
> I'm not sure how much of an efficiency concern there is doing a generic RAV here, rather than something more targeted. I mean I suppose in general the std::move could happen in any subexpression of an init - but I assume the case you're trying to catch here is just the one where the name of the parameter and variable are the same (or very similar - like foo_ and foo, etc) and the user accidentally uses the parameter instead of the variable they moved-in-to?
Yes, this patch is trying to find such accidental uses of parameters in constructor body. I chose constructor, because it's more likely to happen there. It's not deciding based on edit distance -- which sounds like a good idea -- but an exact match. Because it looks like a genuine bug (assuming parameter isn't assigned-to a sensible value again). In fact, whether parameter and member have the same name doesn't matter, if parameter is used after move. But I focused on the case where parameter and member both have the same name, because it's harder to see by looking at the code.

================
Comment at: lib/Sema/SemaDecl.cpp:10783
@@ -10674,1 +10782,3 @@
         DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
+        if (getLangOpts().CPlusPlus11)
+          DiagnoseUseOfMovedParams(*this, FD);
----------------
dblaikie wrote:
> I forget, is CPlusPlus11 true in modes greater than 11? (so we still get the warning in 14, etc?)
Yes, CPlusPlus14 sets CPlusPlus11.

================
Comment at: test/SemaCXX/rval-references.cpp:95
@@ -94,1 +94,3 @@
 }
+
+namespace std {
----------------
dblaikie wrote:
> This test case seems a bit long/verbose - you probably don't need to reproduce a substantial amount of this template machinery to reproduce the warning (a trivial movable type rather than a copy of a substantial amount of unique_ptr would suffice, for example - and there's no need for the extra type traits (remove_ref, etc) work in the faux-move implementation, etc)
Yes, I hated that, too! I've had bigger plans; if it can't move-from but copy-from, then it should be fine! It's not applicable to unique_ptr, but for other types. These tests will change more to address a few more issues.


http://reviews.llvm.org/D10425