-Wunsequenced should warn on this: #include <memory> void f(int *p, std::unique_ptr<int> foo) {} int main() { std::unique_ptr<int> p; f(p.get(), std::move(p)); } (motivated by http://crbug.com/409318)
-Wunsequenced is probably not the right warning flag (the operations are sequenced, but sequenced indeterminately), but this seems like a good warning to have. I've seen similar bugs that look like: my_map[p->foo()] = p.release(); The tricky part is avoiding false positives. I tried doing this with a heuristic "calling a non-const member function is a modification; indeterminately-sequenced modifications and accesses gets a warning" but the false positive rate is extremely high. We could special-case std::move, but it'd be better to implement something more general. Ideas?
One idea is to remember which parameters of a function are converted to rvalue references in the function, and then at the call site consider objects passed in these parameters as dead. This would generalize std::move, but it wouldn't work for your example :-/
As C++11 becomes more common, I think rvalue references may be the way to go. For Chromium's pre-C++11 codebase, I think we should add a custom check for scoped_ptr and base::Passed to the Chromium plugin.
For the p.release() thing in comment 1, maybe functions that are marked warn_unused_result should be considered to poison an object, and accesses to the same object without sequence point should trigger the warning?
*** Bug 27562 has been marked as a duplicate of this bug. ***
From the dupe: """Maybe this could work similar to -Wuninitialized where we conceptually treat things that have been move()d from as uninitialized from that point on""" (yes, nothing guarantees that objects are uninitialized after they're moved from, but in practice they usually are. this is a fairly common problem; we should come up with some approach)
Created attachment 19676 [details] wip I looked at this some a few months ago. Here's what I had. I'm not sure I like the code approach (and it's incomplete), but the test is maybe somewhat useful.
We just hit a similar case and would much appreciate having this warning. In struct foo { void bar(int); }; foo get_foo(int); void g() { int a = 0; get_foo(a).bar(a++); } Clang warns about the "unsequenced modification", but if the int is replaced with a struct that is moved, no warning is issued: #include <utility> struct zed { zed(); zed(const zed&); zed(zed&&); }; struct foo { void bar(zed); }; foo get_foo(zed); void g() { zed a; get_foo(a).bar(std::move(a)); }
clang-tidy got this bugprone-use-after-move check here: https://reviews.llvm.org/D23353 It seems to heavily use AST matchers, which wouldn't be allowed in clang proper. This is a pretty popular feature request, so even if it gets added as an off-by-default warning, I think this would be valuable. Then we could reassess enabling it by default once we better understand the false positive rate.
Sorry for going back and forth, but the standard is particularly hard to read around this. In the case clang does warn: struct foo { void bar(int); }; foo get_foo(int); void g() { int a = 0; get_foo(a).bar(a++); } Isn't https://eel.is/c++draft/expr.call#8 guaranteeing that the "get_foo(a)" is evaluated before "a++"?
(In reply to Rafael Ávila de Espíndola from comment #10) > Isn't https://eel.is/c++draft/expr.call#8 guaranteeing that the "get_foo(a)" > is evaluated before "a++"? Yes; that's a bug in this warning in -std=c++17 and later. SequenceChecker::VisitCallExpr has no idea that this sequencing exists. (But it does know about the other C++17 sequencing changes... I guess we overlooked this one.)
> Isn't https://eel.is/c++draft/expr.call#8 guaranteeing that the "get_foo(a)" > is evaluated before "a++"? The patch for this rule is in D58579. I wrote it at the same time I implemented the other C++17 rules in SequenceChecker but this one has been stuck for a while.