On Jan 24, 2013, at 5:35 PM, Jordan Rose <[email protected]> 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. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
