[cfe-dev] [PATCH] PCH Support for C++ decls

[Douglas Gregor]

Sent from my iPhone

On Mar 3, 2010, at 7:08 AM, Andrew Sutton <andrew.n.sutton at gmail.com>  
wrote:

>
> > Since I noticed Doug commit PCH support for NamespaceDecl's the  
> other day (week?), I thought I'd send an update to my last PCH  
> patch. I've been sitting on this work for quite some time while I've  
> been busy with other things.
> >
>
> Nifty.
>
> 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.
>
> I think we really need better tests. Due to PCH's laziness, it is  
> extremely annoying to debug when it breaks, so small tests for each  
> kind of declaration/statement/expression are important for catching  
> problems quickly.
>
> Yes... it is very annoying to debug :)
>
> We should keep the existing DECL_* constants in order, since each  
> reshuffle is (unnecessary) breakage in backward-compatibility for  
> the PCH format.
>
> I moved all of the constants down to the bottom (and reorganized a  
> couple).

Thanks.

>
> I don't see why we should need this. We already have startDefinition 
> ()/completeDefinition(), which takes care of allocating the  
> DefinitionData and linking it in.
>
> Removed. I was concerned about side effects, but after looking I  
> don't think there are any. I'm still having problems understanding  
> which classes should be serialized. More below.
>
> Please store/load fields in the PCH record in the same order as they  
> occur in the corresponding AST; it makes it easier to verify that  
> everything is there. In this case, BaseOfClass and Virtual should be  
> swapped (the same goes for the reader).
>
> I think I've addressed all instances that you've pointed out, but  
> haven't checked every decl yet. I wish there was an easier way to do  
> this. Maybe I could use clang to dump the order of members...

Well, Clang can clearly parse itself, which means that there is a fun  
(but time-consuming) answer to this.

>
> Why const-qualify the NestedNameSpecifier? You're just  
> const_cast'ing it away anyway.
>
> It's a non-modifying operation. I was only const-casting it away  
> because getPrefix() returns a non-const ptr. It's probably easier  
> just to remove the const in the function that try to make NNS more  
> const-friendly.

A const getPrefix (which does a const_cast internally) would be fine.  
NestedNameSpecifiers are logically immutable anyway.

>
> Why not push all of the NestedNameSpecifiers onto a stack and then  
> emit them in reverse order? That would make it far easier for the  
> reader to reconstruct them properly, since the prefix will come  
> before the nested-name-specifier itself.
>
> This won't work with nested-name-specifiers that have prefixes,  
> because we see the nested-name-specifier before its prefix, and end  
> up trying to build the nested-name-specifier backwards. Use the  
> stack I mentioned previously to address this problem, and you can  
> then get rid of the NNS/P dance, since the last nested-name- 
> specifier you build will be the complete one.
>
> For some reason, I thought that trying to write this without a stack  
> would be a better solution. Other than that no real reason. I'll  
> rewrite this in the next day or two.

SmallVector makes a good stack.
>
> FWIW, I've typically been ordering the Visit functions the same way  
> as the Decl nodes are ordered in DeclNodes.def.
>
> Fair enough. Would it be worthwhile reordering all of the ObjC  
> visitors too?

Not in this patch. We can tweak that later.

>
> Aside: anonymous namespaces are somewhat interesting for PCH,  
> because we need to make sure that an anonymous namespace defined in  
> the source file gets linked to an anonymous namespace defined in the  
> PCH file, since they are effectively the same anonymous namespace.  
> One fun way to test this is:
>
>  // In PCH:
>  namespace {
>    class X;
>  }
>
>  // In source file (that includes PCH):
>  namespace {
>    class X;
>  }
>
> I'll add this to the test tomorrow.
>
> +void PCHDeclReader::VisitCXXRecordDecl(CXXRecordDecl *D) {
> +  VisitRecordDecl(D);
> +  // Injected class names are, I think, actually empty.
>
> Yes, they are. Aside from making sure to get them into the right  
> declaration context and link up their previous declaration chains,  
> there shouldn't be much to serialize for them.
>
> isInjectedClassName() is the wrong check for this. Instead,  
> PCHDeclWriter::VisitCXXRecordDecl should serialize all of the bits  
> in the definition when D->getDefinition() == D, i.e., the  
> declaration you're serializing is the definition.
>
> This is where I'm a little lost w.r.t. class decls, defs, and  
> injected names... Here's my current understanding.
>
> class C;    // call this CXXRecordDecl* decl;
> class C { };  // call this CXXRecordDecl* def;
> // call C::C (the injected name) CXXRecordDecl *inj;
>
> decl->getDefinition() == def
> def->getDefinition() == def
> decl in def->redeclarations() // pseduo python-ese
> inj->getDefinition() == def // ???

Yes

> decl->DefinitionData == def->DefinitionData == inj- 
> >DefinitionData // ???
> decl->DefiniotinData->Definition == def // ???

Yes. An injected-class-name is just a redeclaration with a funny  
scope. And getDefinition always points at the definition.

> And then for a slightly different case, if C isn't defined (so def  
> == inj == 0), then:
> decl->getDefinition() == 0?

Yes.

> Is this about right or is there something I might be missing?

I think you have it correct.

> Also, w.r.t. class serialization. Should virtual bases be serialized  
> the same way that regular bases are serialized? What b

They're all the same. Decls are serialized as a reference to somewhere  
else in the PCH, so it doesn't matter how many times we reference that  
decl.

> +void PCHDeclReader::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
> +  VisitCXXMethodDecl(D);
> +  D->setExplicitSpecified(Record[Idx++]);
> +  if (D->isThisDeclarationADefinition())
> +    D->setImplicitlyDefined(Record[Idx++]);
> +  // FIXME: Serialize member initializers?
> +}
>
> I suggest that the PCH reader/writer avoid cleverness, and serialize  
> exactly what is in the AST (rather than doing the  
> isThisDeclarationADefinition() check).
>
> This is a problem since isImplicitlyDefined and setImplicitlyDefined  
> will assert if isThisDeclarationADefintion returns false. Would it  
> be an acceptable workaround to add non-asserting accessors?

Oh, I missed the assert. In that case, please ignore my comment.

> Back to work...
>
> Andrew Sutton
> andrew.n.sutton at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100303/eadfe5a6/attachment.html>