szepet accepted this revision. szepet added inline comments. This revision is now accepted and ready to land.
================ Comment at: lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp:426 + + State = State->remove<TrackedRegionMap>(WholeObjectRegion); C.addTransition(State); ---------------- szepet wrote: > szepet wrote: > > I am wondering if I made a mistake but I think this should be > > removeFromState() function call. (We should remove every marked subregions > > of the object too.) > > So I suspect a code like this would result a false positive: > > ``` > > struct A{ > > B b; > > void clear(); > > }; > > > > void test(){ > > A a; > > B b = std::move(a.b); > > a.clear(); > > b = std::move(a); //report a bug > > } > > ``` > > > > I mean it is probably a report we do not want to have. > Shame on the test file writer that he does not covered this, though. ^^ Okay, I have checked it and yes, it produces a bugreport (false positive) on the code snippet above (which contains a misspelled variable name so let me write it again): ``` struct B{}; struct A{ B b; void clear(); }; void test(){ A a; B b = std::move(a.b); a.clear(); b = std::move(a.b); //report a bug } ``` In order to eliminate these type of bugs I suggest using here the removeFromState function and move this "WholeObjectRegion algorithm" (the for loop) to the removeFromState function. It is probably out of scope from this patch. Anyway, I accept this since it is great to fix this bug and a good step to a follow-up patch. (Well, if you would like to make these changes in this patch. I'm not gonna hold you back. It is highly appreciated and I would certainly review and all the stuff but I would do it gladly in a follow-up too.) Again, thank you for pointing this out and making the checker more precise! https://reviews.llvm.org/D31538 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits