teemperor accepted this revision. teemperor added a comment. This revision is now accepted and ready to land.
There are several LLDB tests failing with this change. The TestImportBaseClassWhenClassHasDerivedMember.py is probably the only relevant one. The others are probably failing for the same reason but with a more convoluted setup. I looked into the failure and it seems this patch is now skipping the workaround in `ImportContext` that was introduced in D78000 <https://reviews.llvm.org/D78000> (as the `Importer.GetAlreadyImportedOrNull` will give us a DeclContext and then we just use that as-is). If you remove the `if (!DC || !LexicalDC)` before the `if ((Err = ImportDeclContext(D, DC, LexicalDC)))` this should keep the old behaviour. Beside that issue this LGTM. Removing the 'if' seems straightforward, so I'll accept this. Thanks! ================ Comment at: clang/lib/AST/ASTImporter.cpp:2522 + // Add to the lookupTable because we could not do that in MapImported. + Importer.AddToLookupTable(ToTypedef); + ---------------- 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 ================ Comment at: clang/unittests/AST/ASTImporterTest.cpp:5932 + typedef T U; + A(U, T); + }; ---------------- Second parameter seems not relevant for the test? 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
