a_sidorin added a subscriber: spyffe. a_sidorin added a comment. Hi Balazs, The changes look mostly fine to me. I think they can be accepted after some minor stylish fixes.
Hi Shafik, > I think these changes make sense at a high level but I am not sure about the > refactoring strategy. I am especially concerned we may end up in place where > all the effected users of the API don't get updated and we are stuck with > this parallel API. As I understand, these changes are done exactly in order to perform a seamless transition into a new error-checking API. This was discussed in both cfe-dev and lldb-dev: https://lists.llvm.org/pipermail/cfe-dev/2018-April/057771.html. It looks like the patches for the final transition will be ready soon. @balazske, @martong Am I correct? > Tagging in @rsmith since he has reviewed a lot of recent changes involving > ASTImpoter that I have seen recently and he will have a better feeling for > how these types of refactoring on handled on the clang side. I am mostly > focused on the lldb side but care about the ASTImporter since we depend on it. Review from @rsmith is extremely appreciated; however, me and @spyffe have reviewed most ASTImporter-related patches in the last two years. ASTImporter itself doesn't have any use in clang frontend. It was an LLDB bridge only until CSA started to use it too. ================ Comment at: include/clang/AST/ASTImporter.h:244 + /// context, or the import error. + llvm::Expected<NestedNameSpecifier *> Import_New(NestedNameSpecifier *FromNNS); + // FIXME: Remove this version. ---------------- Please split this line and the changed line below. ================ Comment at: lib/AST/ASTImporter.cpp:7882 +Expected<NestedNameSpecifier *> ASTImporter::Import_New(NestedNameSpecifier *FromNNS) { + NestedNameSpecifier *ToNNS = Import(FromNNS); ---------------- Looks like this line needs to be split too. ================ Comment at: lib/AST/ASTImporter.cpp:8254 +Expected<CXXBaseSpecifier *> ASTImporter::Import_New(const CXXBaseSpecifier *From) { + CXXBaseSpecifier *To = Import(From); ---------------- ... and this one too. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53751/new/ https://reviews.llvm.org/D53751 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
