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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
