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@gmail.com