dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.

This looks good to me for a quick fix that we plan to address in a more 
principled fashion later.

However, I'd like you to add a comment at each of the places where you use the 
parent map to note something like  "This is a quick dirty fix and we really 
shouldn't be using the parent map to model behavior." I understand why you're 
doing it, but I'd like to make sure that someone reading this code doesn't 
think that it is a good pattern to use the parent map and decide to use it 
elsewhere!

Using the parent map is slow and is a sign that our reasoning isn't 
compositional. The analyzer should be able to figure out the correct behavior 
based on the expression and its state/context alone. The fact that we need the 
parent map here is an indication that we don't have the right representation 
for constructors in the analyzer. (I know you're planning on tackling this in 
the future!)


https://reviews.llvm.org/D40841



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to