[PATCH] Analyzer: model trivial copy constructors directly

[Anna Zaks]

On Jan 24, 2013, at 5:35 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> Hi, Anna. This is the analyzer patch I was working on: modeling the evaluation of a trivial copy constructor with a bind. The idea is pretty straightforward, but I wondered if you had any opinions on the design. Does it make sense to do this as a replacement for defaultEvalCall in VisitCXXConstructExpr? Is there another place to fit it in that would make more sense?
> 
> (Part of why I'm hesitant is because this is modifying Core when it could just be a core checker in theory. That requires talking about switching to evalCall(CallEvent), though, instead of the current limited evalCall(CallExpr). In practice, I don't think checkers can properly model a user-level bind right now, and using BodyFarm wouldn't work because you can't copy structs by value in C++.)
> 

I think it's fine to implement this in the core. That is where the C++ semantics are being interpreted now.

> Besides making it cheaper to process these constructors (which in C don't even have the preCall/postCall overhead), this eliminates the spurious warning for copying a not-fully-initialized POD struct, which would match our C behavior. (It's not such an uncommon thing to do.)
> 
> CGPoint p;
> p.x = 0;
> CGPoint p2 = p; // no-warning
> 
> This is <rdar://problem/12305288>. If we don't want to do this at all, though, I'll just not emit the warning when in a trivial copy-or-move constructor.
> 
> What do you think?
> Jordan
> 
> <trivial-copy-ctors.patch>

+  /// Models a trivial copy or move constructor call with a simple bind.
Does this deal with move constructors as well? Are there test cases?

Could you add a comment on when this if statement is needed and when it is not needed?
+  if (const Loc *L = dyn_cast<Loc>(&V))
+    V = Pred->getState()->getSVal(*L);

+    // Note that we have to be careful here because constructors embedded
+    // in DeclStmts are not marked as lvalues.
+    if (!E->isGLValue())
+      if (const MemRegion *MR = ThisV.getAsRegion())
+        if (isa<CXXTempObjectRegion>(MR))
I do not fully understand what the comment means and why the test for CXXTempObjectRegion is needed (if I comment it out all tests pass). So maybe better comment and more tests?

+  ExplodedNodeSet Dst;
+  Bldr.takeNodes(Pred);
:( You could possibly just pass Src and Dst to performTrivialCopy to avoid this, but I don't think there is a great solution here other than finishing transitioning to Builders everywhere.