martong added inline comments.
================ Comment at: lldb/source/Symbol/StdModuleHandler.cpp:242 + // Instantiate the template. + found_decl = ClassTemplateSpecializationDecl::Create( + m_sema->getASTContext(), ---------------- martong wrote: > teemperor wrote: > > martong wrote: > > > martong wrote: > > > > Is there any guarantee that the before any subsequent > > > > clang::ASTImporter::Import call this newly created node is registered > > > > as imported (via ASTImporter::MapImported)? > > > > > > > > The reason why I am asking this is because we have to enforce in > > > > clang::ASTImporter that after every Decl is created then immediately it > > > > is registered as an imported Decl, see > > > > `ASTImporter::GetImportedOrCreateDecl`. This we we make sure that no > > > > subsequent import calls will create the same node again. > > > > The cycles in the AST are handled properly only this way. > > > > > > > > This is one reason which makes me worried about exposing the import > > > > mechanism via `ImportInternal`. Would it be working if > > > > `GetImportedOrCreateDecl` was virtual and overridden here? > > > > Would it be working if GetImportedOrCreateDecl was virtual and > > > > overridden here? > > > > > > Sorry, that would certainly not work, but perhaps a virtual hook inside > > > that function could work... > > Sorry for the late reply. > > > > So I was looking into changing the code to your suggestion, but I don't > > think we can get this working reliably. The problem is that if we intercept > > the importing in GetImportedOrCreateDecl then we need to intercept not just > > `std::vector` but also all related imports that the ASTImporter may make > > before importing `std::vector` itself, e.g. DeclContext, parent classes, > > return type, template args, etc. Also we constantly would need to update > > this list in case the standard library or the ASTImporter change internally > > to make sure we intercept everything. > > > > Maybe we could instead implement an mechanism in the LLDB code that ensures > > we do call MapImported and add the decl to the lookup directly after > > creating a decl? E.g. by implementing something similar to ASTImporter's > > `GetImportedOrCreateDecl`? > Yes, I think that could work. > So, we could have a new function: > ``` > void CreateDeclPostamble(Decl *FromD, Decl* ToD) { // better naming? > // Keep track of imported Decls. > Importer.MapImported(FromD, ToD); > Importer.AddToLookupTable(ToD); > InitializeImportedDecl(FromD, ToD); > } > ``` > And we'd call this from `GetImportedOrCreateDecl` and here after `::Create`. Well, the thing is a bit more complex because this is in `ASTNodeImporter`, so we need to wire things up to `ASTImporter`. But that seems doable. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59537/new/ https://reviews.llvm.org/D59537 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits