Hi Jordy, Ted<div><br></div><div>Thanks for the appreciation and for comments. Some comments inline:</div><div><br><div class="gmail_quote">W dniu 18 sierpnia 2010 07:47 użytkownik Jordy Rose <span dir="ltr"><<a href="mailto:jediknil@belkadan.com">jediknil@belkadan.com</a>></span> napisał:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><br>
Wow, good work.<br>
<br>
I agree with most of what Ted said. A few additional comments:<br>
<br>
- I like the single enum rather than kind and subkind, but there are 7<br>
elements, not 6. So it won't fit in a PointerIntPair anyway, though having<br>
a Stmt* field (or void*) and an enum field would be preferable to two<br>
PointerIntPairs.<br>
I'm still not totally convinced we need scope markers in the CFG, though.<br>
<br></blockquote><div>For both destructor kinds I will need space for two pointers. For automatic object I need it's declaration and statement that cause destructor to be called. For temporary object I need expression bound to the temporary and the full expression that contains the temporary.</div>
<div><br></div><div>I could allocate separate struct for those and point to it from CFGElement. If we drop the scopes and move information about kind of destructor to this struct, then CFGElement will need only one PointerIntPair.</div>
<div><br></div><div>If we would want scopes modeled we could use same struct as for destructors. So we would end up with something like this:</div><div><br></div><div>- Statement and LValStatement with pointer to Stmt*,</div>
<div>- Initializer with pointer to CXXBaseOrMemberInitializer*,</div><div>- Extendend CFGElement with pointer to struct with information about destructor or scope.</div><div><br></div><div>Our kind enum would look like this:</div>
<div><span class="Apple-style-span" style="font-family: arial, sans-serif; font-size: 13px; border-collapse: collapse; ">enum Kind {<br>  Statement,<br>  LvalStatment,<br>  BEGIN_STATEMENT = Statement,<br>  END_STATEMENT = LvalStatement,<br>
  Initializer,</span></div><div><span class="Apple-style-span" style="font-family: arial, sans-serif; font-size: 13px; border-collapse: collapse; "><br></span></div><div><span class="Apple-style-span" style="font-family: arial, sans-serif; font-size: 13px; border-collapse: collapse; ">  ExtendedElement, // This would be internal, client would never get this</span></div>
<div><span class="Apple-style-span" style="font-family: arial, sans-serif; font-size: 13px; border-collapse: collapse; ">                              // with getKind() method.</span></div><div><span class="Apple-style-span" style="font-family: arial, sans-serif; font-size: 13px; border-collapse: collapse; "><br>
  AutomaticObjectDtor,<br>  TemporaryObjectDtor,<br>  BEGIN_DTOR = AutomaticObjectDtor,<br>  END_DTOR = TemporaryObjectDtor,<br>  StartScope,<br>  EndScope,<br>  BEGIN_SCOPE = StartScope,<br>  END_SCOPE = EndScope<br>};</span></div>
<div><span class="Apple-style-span" style="font-family: arial, sans-serif; font-size: 13px; border-collapse: collapse; "><br></span></div><div><span class="Apple-style-span" style="font-family: arial, sans-serif; font-size: 13px; border-collapse: collapse; ">This way we will have space efficient CFG for Obj-C and C languages. For C++ CFG will occupy less memory if there will be more statement then destructor CFGElements.</span></div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
- Why don't we make this a static type hierarchy like SVal or MemRegion?<br>
We'd have CFGStmt, CFGInitializer, CFGDestructor, and CFGScope(Marker)<br>
inheriting from CFGElement. That way we can have, among other things, a<br>
CFGStmt that simplifies to a Stmt. Instead of asserts, we can just have the<br>
type-specific methods on the subclasses.<br>
<br></blockquote><div>Yes, this will be much better approach. I will change this.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
- In UnreachableCodeChecker::getUnreachableStmt(), isn't the block<br>
guaranteed to have an initial statement for the same reason as in<br>
ReachableCode.cpp?<br>
<br></blockquote><div>Do you mean that if block is unreachable it will start with a statement and not with initializer, destructor or scope?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

- Very tiny comment: most of the LLVM codebase has no indentation on blank<br>
lines.<br>
<br></blockquote><div>I will try to follow this convention then.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Great to get more C++ support in the analyzer!<br>
<font color="#888888">Jordy<br>
</font></blockquote></div><br></div><div>Cheers,</div><div>Marcin</div>