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

Reply via email to