martong marked an inline comment as done. martong added inline comments.
================ Comment at: clang/lib/AST/ASTImporter.cpp:2522 + // Add to the lookupTable because we could not do that in MapImported. + Importer.AddToLookupTable(ToTypedef); + ---------------- 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.) 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
