<br><br><div class="gmail_quote">On Mon, Jun 4, 2012 at 7:22 PM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
On Jun 4, 2012, at 1:43 , Daniel Jasper <<a href="mailto:djasper@google.com">djasper@google.com</a>> wrote:<br>
> Thanks for coming up with this!! I propose a different solution which utilizes the fact that friend relationships are not transitive. Thus, if a class has a friend, it only needs to check whether all methods and nested classes of the friend are defined but does not need to consider the friend's friends.<br>

<br>
</div>Oh, of course! Good catch.<br>
<div class="im"><br>
> Could you take one last look as the implementation of IsRecordFullyDefined has changed substantially?<br>
<br>
</div>Thank you for being careful. :-) Only small comments this time, I think:<br>
<br>
- Please attach the & to the variable name for references, per LLVM style conventions (same as pointers).<br>
<br>
- You've got an isa followed by dyn_cast for CXXMethodDecl; please change to just dyn_cast (like CXXRecordDecl).<br>
<br>
- DenseMap has an operator[], which is a little easier to read than insert+make_pair.<br>
<br>
- This is very nitpicky, but the style guide says to put all comments on their own line, and to end them with periods (full stops). So "// This is a template friend, give up" should be reformatted as such.<br>
<br>
Also, it took me a while to convince myself that CXXRecordDecls coming out of getFriendDecl() are also problematic, but what convinced me is that if they don't have a TypeSourceInfo, we can't possibly see their definition. It does seem like it is possible to have CXXRecordDecls come out of getFriendDecl(), though, and that might be worth commenting.<br>
</blockquote><div><br></div><div>I have never managed to get a CXXRecordDecl out of getFriendDecl(). For friend classes getFriendDecl() always seems to return 0. How do I create such a case?</div><div><br></div><div>Also, it uses FriendUnion as internal data type (<a href="http://clang.llvm.org/doxygen/DeclFriend_8h_source.html#l00041">http://clang.llvm.org/doxygen/DeclFriend_8h_source.html#l00041</a>), which hints at the result either being a NamedDecl for friend functions or a TypeSourceInfo for friend classes. I have added comments and an else-branch to the "if(..->getAsCXXRecordDecl())" so that if we don't understand the FriendDecl, we mark the class as incomplete.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'm tempted to say that passing around a pair of maps to two different methods is worth a private class, but I guess it's fine the way it is.<br>
<br>
I think that's it. Good work!<br>
<span class="HOEnZb"><font color="#888888"><br>
Jordan<br>
<br>
</font></span></blockquote></div><br>