[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Aleksei Sidorin via Phabricator via cfe-commits]

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