<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Aug 18, 2010, at 12:11 AM, Marcin Świderski wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">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></div></blockquote><div><br></div><div>Repeating what I said in my reply to Jordy, I think it's fine to burn an extra pointer if it gives us flexibility to track more information that would be useful for diagnostics.</div><br><blockquote type="cite"><div><div class="gmail_quote">
<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></div></blockquote><div><br></div><div>That is also a possibility. It could be allocated from the BumpPtrAllocator.</div><br><blockquote type="cite"><div><div class="gmail_quote">
<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></div></blockquote><div><br></div><div>What is the purpose of the 'ExtendedElement' kind?</div><br><blockquote type="cite"><div><div class="gmail_quote">
<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-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto; ">
- 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></div></blockquote><div><br></div>Sounds great.</div><div><br><blockquote type="cite"><div><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto; ">
- 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><div><br></div><div>Yes, an unreachable block could certainly start with an initializer if it had no initializer argument and the previous initializer called a no-return function. Gross, but I guess possible.</div></div><br><div>Moreover, a call to builtin_unreachable is not guaranteed to be the first CFGElement in a CFGBlock. I think the checker's logic is likely wrong here. It should be iterating over the block, looking for this specific CallExpr.</div></body></html>