dang added inline comments.
================ Comment at: clang/lib/ExtractAPI/DeclarationFragments.cpp:642-648 +// Instantiate template for FunctionDecl. +template FunctionSignature +DeclarationFragmentsBuilder::getFunctionSignature(const FunctionDecl *); + +// Instantiate template for ObjCMethodDecl. +template FunctionSignature +DeclarationFragmentsBuilder::getFunctionSignature(const ObjCMethodDecl *); ---------------- Do we need to explicitly instantiate them? Wouldn't they get instantiated as needed? ================ Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:245 + if (const auto *SuperClassDecl = Decl->getSuperClass()) { + SuperClass.Name = SuperClassDecl->getObjCRuntimeNameAsString(); + SuperClass.USR = API.recordUSR(SuperClassDecl); ---------------- dang wrote: > zixuw wrote: > > dang wrote: > > > Shouldn't we be recording this string in the StringAllocator for reuse > > > later. I have a patch in progress for macro support that will do the > > > serialization once the AST isn't available anymore so we really shouldn't > > > be taking in StringRefs for stuff in the AST. > > Right but that only makes sense once we kill the AST before serialization > > right? I mean I'm happy to wrap these in `copyString` now if we know for > > sure that we're doing that in a later patch. But it feels beyond the scope > > of this particular patch. Especially that we'd also need to go through the > > previous code to copy the names of global records, enums, etc. anyway. > > > > I feel like that the change to surface `APISet` and punt serialization > > should be separated out from the macro change. And we can wrap all the name > > strings in that patch so that it's a meaningful atomic commit. > Yeah sounds good! I have looked into it a bit more and the ASTContext will still be live at that point (it gets deallocated right after we will be doing the serialization) so this is fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122446/new/ https://reviews.llvm.org/D122446 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits