[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added a comment.

Thanks for the review guys!



================
Comment at: clang/lib/AST/ASTImporter.cpp:2522
+  // Add to the lookupTable because we could not do that in MapImported.
+  Importer.AddToLookupTable(ToTypedef);
+
----------------
teemperor wrote:
> martong wrote:
> > shafik wrote:
> > > I am not super excited about this solution, I feel like several bugs we have had can be attributed to these exception control flow cases that we have in the ASTImporter. I don't have any immediate better solution.
> > > 
> > > 
> > Before uploading this patch I had been thinking about several other solutions
> > but all of them had some problems:
> > 
> > 1) 
> > Detect the loop in the AST and return with UnsupportedConstruct. We could do
> > the detection with the help of ImportPath. However, I realized this approach is
> > way too defensive and removes the support for an important AST node which is
> > against our goals.
> > 
> > 2) 
> > Try to eliminate the looping contruct from the AST. I could do that for some
> > cases (D92101) but there are still cases when the Sema must create such a loop
> > deliberately (the test case in this patch shows that).
> > 
> > It is essential to add a newly created Decl to the lookupTable ASAP because it
> > makes it possible for subsequent imports to find the existing Decl and this way
> > avoiding the creation of duplicated Decls. This is why AddToLookupTable is
> > called in MapImported. But the lookuptable operates on the DeclContext of the
> > Decl, and in this patch we must not import the DeclContext before.
> > Consequently, we have to add to the loopkuptable once the DeclContext is
> > imported. Perhaps, we could provide an optional lambda function to
> > GetImportedOrCreateDecl which would be called before MapImported (and that
> > lambda would do the DC import), but this seems even more clumsy.
> > 
> > BTW, the lookuptable is not used in LLDB, there you use the traditional lookup
> > implemented in DeclContext (addDeclInternal and noload_lookup). One problem
> > with noload_lookup is that it does not find some Decls because it obeys to C++
> > visibility rules, thus new duplicated Decls are created. The ASTImporter must
> > be able to lookup e.g. template specialization Decls too, in order to avoid
> > creating a redundant duplicated clone and this is the task of the lookupTable.
> > I hope one day LLDB would be able to switch to ASTImporterLookupTable from
> > noload_lookup. The other problem is with addDeclInternal: we call this later,
> > not in MapImported. The only responsibility attached to the use of addDeclInternal 
> > should be to clone the visibility as it is in the source context (and not managing 
> > the do-not-create-duplicate-decls issue).
> > 
> > So yes, there are many exceptional control flow cases, but most of them stems
> > from the complexity of trying to support two different needs: noload_lookup and
> > minimal import are needed exceptionally for LLDB. I was thinking about to
> > create a separate ASTImporter implementation specifically for CTU and LLDB
> > could have it's own implementation. For that we just need to create an
> > interface class with virtual functions. Having two different implementations
> > could save us from braking each others tests and this could be a big gain, WDYT?
> > (On the other hand, we'd loose updates and fixes from the other team.)
> > 
> > I hope one day LLDB would be able to switch to ASTImporterLookupTable from noload_lookup.
> 
> Done here: https://reviews.llvm.org/D92848
Thanks, this could enable some simplifications in the ASTImporter then.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92209/new/

https://reviews.llvm.org/D92209