<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>First off, what do people think about turning this on by default, or at least making it part of -Wunused? Daniel's being conservative by leaving it off, but you know what people say about off-by-default warnings...</div>
</div></blockquote><div><br></div><div>It would be my intention to turn it on by default (not sure whether by default, as part of Wunused or just as default for LLVM compilations), but I though we should clean up LLVM's codebase before doing so. But I am not sure what the right approach is here.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>---</div><div><br></div><div>As for the patch…sorry, one last problem.</div>
<div><br></div><div><div>+    // Mark as fully defined for now to stop recursion in case of mutual</div><div>+    // friends.</div><div>+    FullyDefined.insert(RD);</div></div><div><br></div><div>I thought about this some more, and it seems like this will cause incorrect warnings:</div>
<div><br></div><div>class B;</div><div>class A {</div><div>  int x;</div><div>public:</div><div>  friend class B;</div><div>  doSomethingWithX();</div><div>}</div><div><br></div><div>class B {</div><div>  int y;</div><div>
public:</div><div>  friend class A;</div><div>}</div><div><br></div><div>When looking through A's decls (because of A::x), we'll come across "friend class B". We'll look through B's decls and find "friend class A". A has already been marked complete, so we mark B complete. Then we go back to A and find "doSomethingWithX()", and A is now (correctly and finally) marked incomplete. When we get to B::y, though, we're in trouble, because B has already been marked as complete via A!</div>
<div><br></div><div>The usual thing to do here is add an "in progress" set, but I'm still not sure exactly how to use that for warnings. I think the best thing to do (and possibly the most correct thing to do) is to simply give up when we see mutual friends declarations -- that is, tentatively mark classes as not fully defined instead of fully defined. That gives us a conservative set of warnings, even if we miss a few valid cases.</div>
</div></blockquote><div><br></div><div>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. I have changed the implementation and added corresponding test cases (see attached new patch).</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class="im"><blockquote type="cite"><div style="word-wrap:break-word"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Finally, even though there's not a big hit traversing the fields at the end, we probably still shouldn't pay for this if the diagnostic is off. Can you check for that before inserting into UnusedPrivateFields, or even doing all the tests?<br>

</blockquote><div><br></div><div>I am now checking whether the diagnostic is enabled before inserting into the set and before doing the checks at the end of the translation unit. Checking before removing from the (then empty) set probably does not give any benefit. I am not sure whether the check on the initializer arguments is expensive (especially the call to HasSideEffects). What do you think?</div>
</div></div></blockquote><div><br></div></div><div>Let's leave it as is. Looking up a diagnostic isn't /that/ cheap, and most initializers are something simple like a VarDecl, a constant, or a FunctionCall. </div>
</div></div></blockquote><div><br></div><div>Ok. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>I think once you've fixed the friend problem you can go ahead and commit. Thanks for working on this!</div>
<span class="HOEnZb"><font color="#888888"><div>Jordan</div></font></span></div></div></blockquote><div><br></div><div>Could you take one last look as the implementation of IsRecordFullyDefined has changed substantially?</div>
<div><br></div><div>Thank you!</div><div>Daniel</div></div><br>