Hi, Anna, Ted<div><br></div><div> Thanks for your link. Attached is another refactoring, using only GraphTraits to implement all this functionality. Please let me know your comments.</div><div>--</div><div>Guoping<br><br>
<div class="gmail_quote">2011/11/12 Anna Zaks <span dir="ltr"><<a href="mailto:ganna@apple.com">ganna@apple.com</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word">I was thinking of something similar to PathDiagnostic.h:523 (<a href="http://clang.llvm.org/doxygen/PathDiagnostic_8h_source.html" target="_blank">http://clang.llvm.org/doxygen/PathDiagnostic_8h_source.html</a>)<span class="HOEnZb"><font color="#888888"><div>
<br></div></font></span><div><span class="HOEnZb"><font color="#888888">Anna.</font></span><div><div class="h5"><br><div><div>On Nov 12, 2011, at 6:23 PM, Guoping Long wrote:</div><br><blockquote type="cite"><div>Anna, </div>
<div><br></div><div>Sounds like a good idea to provide a custom iterator for CFG. But how to achieve that?</div>CFG iterator is defined as BumpVector<CFGBlock*>::iterator, do you mean makes a custom iterator as BumpVector<CFGBlock>::iterator? It's easy to implement begin()/end(), but how to implement ++ operator? All CFGBlocks are stored as CFGBlock* in BumpVector, do you mean also maintaining another vector of CFGBlocks, ex: BumpVector<CFGBlock>? That will also be very messy.<div>
<br></div><div>--</div><div>Guoping<br><br><div class="gmail_quote">2011/11/12 Anna Zaks <span dir="ltr"><<a href="mailto:ganna@apple.com" target="_blank">ganna@apple.com</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word">Hi Guoping,<div><br></div><div>I propose to use a 3d solution.</div><div><br></div><div>What we need is to change the clang::CFG GraphTraits to have the nodes_iterator type consistent with that of Function's GraphTraits. As I pointed out in one of the previous emails, they are currently not consistent even in terms of NodeType (which is a type internal to traits) - one iterates over NodeType and another over NodeType*. This can be implemented by adding a a custom iterator for CFG which returns CFGBlock* (instead of CFGBlock**). Then, you define nodes_iterator as your new iterator.</div>
<div><br></div><div>Cheers,</div><div>Anna.</div><div><div><div><div><div>On Nov 12, 2011, at 1:35 PM, Guoping Long wrote:</div><br><blockquote type="cite">Hi, Anna<div><br></div><div> I am doing the refactoring only using GraphTraits. Here is the problem: We still need to deal with the difference between the iterator implementation of CFG and Function. The fundamental reason for this difference is that CFG used BumpVector, while the Function class used iplist. There are two possible solutions to this problem:</div>
<div><br></div><div> 1. As you pointed out, let's change the CFG design to use iplist instead of BumpVector. But for me, this may seem unnecessarily intrusive to Clang.</div><div> 2. In the GraphTraits class, let's provide a convert routine to do the iterator to Block conversion. For example, in GraphTraits<CFG*>, let's add this:</div>
<div> static NodeType *I2B(node_iterator I) {</div><div> return *I; </div><div> }</div><div> In this way, we can still conceal all implementation details in GraphTraits. But this may make the GraphTraits a little messy. Please let me know your comments. Which approach should be better?</div>
<div><br></div><div>----</div><div>Guoping</div><div><br><div class="gmail_quote">2011/11/8 Guoping Long <span dir="ltr"><<a href="mailto:longguoping@gmail.com" target="_blank">longguoping@gmail.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi, Ted, Anna<div><br></div><div> Thanks. Now I think I understand the design rationale behind GraphTraits. I will do another refactoring later this week.</div><span><font color="#888888"><div><br></div><div>
--</div></font></span><div><span><font color="#888888">Guoping<br><br></font></span><div class="gmail_quote"><span><font color="#888888">
2011/11/7 Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span><br></font></span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div><div style="word-wrap:break-word">
<div><div><div><div>On Nov 7, 2011, at 9:10 PM, Anna Zaks wrote:</div><br><blockquote type="cite"><div style="word-wrap:break-word"><br><div><div>On Nov 7, 2011, at 7:08 PM, Ted Kremenek wrote:</div><br><blockquote type="cite">
<div style="word-wrap:break-word"><div><div>On Nov 3, 2011, at 10:50 AM, Ted Kremenek wrote:</div><br><blockquote type="cite"><div style="word-wrap:break-word"><div><div>On Nov 2, 2011, at 4:23 PM, Anna Zaks wrote:</div>
<br>
<blockquote type="cite"><span style="border-collapse:separate;font-family:Helvetica;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;font-size:medium">On llvm's side, it would involve changing the recalculate function to use getEntryBlock instead of front(). Looks like they are the same thing.<br>
<br>On Clang's side, we could just rename CFG::getEntry() -> CFG::getEntryBlock().<br></span></blockquote></div><br><div>Both of these seem reasonable to me.</div></div><br></blockquote><br></div><div>Looking back at this comment, I think the Dominators algorithm should not rely on the existence of a 'getEntryBlock()' and really just being using GraphTraits that can get the entrance of the graph. We shouldn't have to rename anything in the CFG just to get the dominators analysis to work. If we are missing a concept, that is one thing, but renaming something means that the dominators analysis is relying on particular methods being present. That just feels really wrong to me. As I mentioned in another email I just sent, there is also no reason for the dominators analysis to have any notion of "basic blocks" either since it is a generic graph algorithm.</div>
</div></blockquote></div><br><div>Ted, this comment is from before Guoping pointed out that this function name is not the only inconsistency between Function and CFG (at that point it seemed fine to just make a simple change to get the patch through). With the new approach, where we change the dominators to use GraphTraits, it will handle this case as well. There is getEntryBlock() method in the GraphTraits.</div>
</div></blockquote></div><br></div></div><div>Indeed. I figured this would be handled by using GraphTraits for everything, but I wanted to amend my previous comment for posterity. :)</div></div><br></div></div><div>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu" target="_blank">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
<br></div></blockquote></div><br></div>
</blockquote></div><br></div>
</blockquote></div><br></div></div></div></div></blockquote></div><br></div>
</blockquote></div><br></div></div></div></div></blockquote></div><br></div>