nickdesaulniers added inline comments.
================ Comment at: clang/lib/CodeGen/CGDecl.cpp:103-118 + case Decl::CXXRecord: // struct/union/class X; [C++] + if (CGDebugInfo *DI = getDebugInfo()) + if (CGM.getCodeGenOpts().hasMaybeUnusedDebugInfo() && + cast<CXXRecordDecl>(D).hasDefinition()) + DI->completeUnusedClass(cast<CXXRecordDecl>(D)); + return; case Decl::Record: // struct/union/class X; ---------------- dblaikie wrote: > All of these might be able to be collapsed together - using > "cast<TagDecl>(D).hasDefinition()" ... to have a common implementation. `TagDecl` doesn't have a method `hasDefinition`. Only `CXXRecordDecl` does. ================ Comment at: clang/lib/CodeGen/CGDecl.cpp:116 + if (CGDebugInfo *DI = getDebugInfo()) + if (cast<EnumDecl>(&D)->getDefinition()) + DI->EmitAndRetainType(getContext().getEnumType(cast<EnumDecl>(&D))); ---------------- dblaikie wrote: > I think the right thing to call here (& the other places in this patch) is > "hasDefinition" rather than "getDefinition" (this will avoid the definition > being deserialized when it's not needed yet (then if we don't end up emitting > the type because the flag hasn't been given, it won't be deserialized > unnecessarily)) `TagDecl` doesn't have a method `hasDefinition`. Only `CXXRecordDecl` does. ================ Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5385 + if (getCodeGenOpts().hasMaybeUnusedDebugInfo() && CRD->hasDefinition()) + DI->completeUnusedClass(*CRD); + else if (auto *ES = D->getASTContext().getExternalSource()) ---------------- dblaikie wrote: > I think this might not work as intended if the type trips over the other > class deduplication optimizations (try manually testing with a type that has > an external VTable?) - and perhaps this codepath should use the EmitAndRetain > functionality. completeUnusedClass eventually calls into CGDebugInfo::completeClass which makes use of the TypeCache. What does it mean for a class to have "an external vtable?" Can you give me an example? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80242/new/ https://reviews.llvm.org/D80242 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
