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

Reply via email to