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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
