[PATCH] D51633: [ASTImporter] Added error handling for AST import.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


================
Comment at: lib/AST/ASTImporter.cpp:194
+      // FIXME: This should be the final code.
+      //auto ToOrErr = Importer.Import(From);
+      //if (ToOrErr) {
----------------
a_sidorin wrote:
> Do I understand correctly that we have to use the upper variant because ASTImporter API is unchanged?
The "upper variant" (`importInto(ImportT &To, const ImportT &From)` form) is the final one. It is only to make the code more simple, you can call `importInto` in ASTImporter instead of `Importer.importInto`. This second variant is needed because the ASTImporter API is not changed (yet), the `Import` functions still return pointer (later they will return `Expected`).
But this special version for pointer is still needed to have type cast for pointers. The commented-out code should be used later.


================
Comment at: lib/AST/ASTImporter.cpp:1663
+      // Ignore the error, continue with next Decl.
+      consumeError(ImportedOrErr.takeError());
+  }
----------------
a_sidorin wrote:
> Silent fail doesn't look correct in case of structures. Should we add a FIXME?
Maybe yes. But aborting the whole import of the DeclContext could cause lots of not imported things. A warning can be printed but maybe it is already printed at the called failed import. There was no FIXME in the old code either.


Repository:
  rC Clang

https://reviews.llvm.org/D51633