<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Mar 8, 2010, at 7:44 AM, Andrew Sutton wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div class="gmail_quote"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div bgcolor="#FFFFFF"><div class="im"><blockquote type="cite"><div>
<div class="gmail_quote"><div>I've spent the morning addressing issues in the patch and had some questions, comments. I'm not sending up a new patch just yet, though.<br></div></div></div></blockquote></div></div>
</blockquote><div>I had planned to send this up on Saturday, but couldn't connect to the server in the morning so I worked on other stuff instead. Attached is a new version of C++/PCH Decl support. I think I've addressed all of your comments. I've also added support for<br>
<br>- unresolved value/type decls<br>- friend type references<br>- ctor member initializers<br>- stubbed a function for friend template decls<br></div></div></blockquote><div><br></div><div>It would be much easier to handle such additions as separate patches once the main patch has gone in. Big patches take much longer to review. We haven't quite dealt with all of the issues there yet.</div><div><br></div><div>Also, friends are in a state of flux, so it's not worth trying to (de-)serialize them now. Once the AST settles down and access control is turned on by default, it will make sense to implement PCH support with friends.</div><br><blockquote type="cite"><div class="gmail_quote"><div>There are 3 issues with this patch that I either don't like or are cause problems.<br>
<br>1. I needed a way to attach the DefinitionData of a CXXRecordDecl that is not a definition to a previously defined record declaration via the PCH reader (hence the attachDefinition()) function in CXXRecordDecl. It basically addresses this problem:<br>
<br>class C { };<br>class C; // DefinitionData not attached during reading<br></div></div><br>The DefinitionData isn't attached but the previous declaration is set correctly. IIRC, the same situation happens with injected class names.<br>
<br>I'm not thrilled with this solution. I think the best solution would be to make setPreviousDeclaration automatically share (forward?) DefinitionData. I just didn't want to open that can of worms in this patch.<br></blockquote><div><br></div><div>Since you posted the original patch, a better solution has come up. When we start a definition of a CXXRecordDecl, we go back and fix up all of the DefinitionData pointers.</div><br><blockquote type="cite">2. Anonymous namespaces within other namespaces appear to be incorrectly linked in semantic analysis but not PCH serialization. Of course, my interpretation of C++ semantics may also be wrong :) Let's say we have:<br>
<br>// Yes, there are two of them<br>namesapce N { namespace { class C; } } // 1<br>namesapce N { namespace { class C; } } // 2<br>
N::C* p; // ERROR ambiguous lookup for C.<br><br>My understanding is that the anonymous namespace should be linked so that 1 and 2 refer to the same class. Is that right? I added a test to SemaCXX/namespaces.cpp to check.<br></blockquote><div><br></div><div>Yes, good catch. I ended up fixing this as part of PR6341.</div><br><blockquote type="cite">3. There seems to be a bug in the PCH reader when you explicitly initialize member variables of a base class. Let's say you have:<br><br>struct B : A {<br> B() : A(), x() { } // <- ASSERT<br> int x;<br>};<br>
<br>The assertion occurs in the deserialization (ReadStmt) of the ImplicitValueInit expr for x(). Somehow, after visiting that stmt/expr, the Idx == Record.size() - 1. Curiously, if B has no base classes, this seems to work fine.<br>
<br>I'm not sure what the status of this patch is going to be since a) clang/llvm seems to be in a code freeze and b) this patch introduces an assertion. I have a lot of other things to take care of so I may not be able to address those issues in a timely manner.<br></blockquote><br></div><div>Only the 2.7 branch of Clang/LLVM is in code freeze. The trunk is in its usual frenzy of activity.</div><div><br></div><div>Overall, this patch isn't quite ready to go in. I've attached an updated version of the patch, which brings your patch into line and fixes some of the issues. It cleans up the handling of DefinitionData (#1), eliminates the code for friend type references (which have changed, and may change yet again), and takes a step further toward getting conversion functions right. However, to get conversion functions 100% correct, the list of conversion functions needs to be loaded lazily: see the FIXME I added in PCHDeclReader::VisitCXXRecordDecl.</div><div><br></div><div>I think fixing conversion functions and the ctor-initializers are crucial for this patch to go in.</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div><div><br></div></body></html>