[PATCH] D31538: [analyzer] MisusedMovedObjectChecker: Fix a false positive on state-resetting a base-class sub-object.

[Peter Szecsi via Phabricator via cfe-commits]

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