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

[Andrew Sutton]

> > 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).


> 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...


> 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.


> 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.


> 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?


> 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 // ???
decl->DefinitionData == def->DefinitionData == inj->DefinitionData // ???
decl->DefiniotinData->Definition == def // ???

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

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

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

+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?

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/89152224/attachment.html>