[PATCH] D62373: [ASTImporter] Store import errors for Decls

[Gabor Marton via Phabricator via cfe-commits]

martong marked 3 inline comments as done.
martong added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:5109
     } else { // ODR violation.
       // FIXME HandleNameConflict
+      return make_error<ImportError>(ImportError::NameConflict);
----------------
a_sidorin wrote:
> Is this FIXME obsolete now?
That is not obsolete.  We should call `HandleNameConflict` as we do in other visit functions.  But it is not trivial how to do that, there are open questions which I cannot address in this PR: 
* Should we pass `D->getDeclName` or rather the name of the imported `ClassTemplateDecl` or should we import the name here (again)? 
* Should we pass the template args to `HandleNameConflict`?


================
Comment at: clang/lib/AST/ASTImporter.cpp:7823
+    auto Pos = ImportedDecls.find(FromD);
+    if (Pos != ImportedDecls.end()) {
+      // Import failed after the object was created.
----------------
a_sidorin wrote:
> I see the enabled test case, but does it cover the logic in this block?
Right, that is not strictly related. I have added relevant unit test cases for the error handling. This branch is handled in `ErrorHappensAfterCreatingTheNodeButBeforeLinkingThatToTheAST` test.


================
Comment at: clang/lib/AST/ASTImporter.cpp:7851
+    if (!getImportDeclErrorIfAny(FromD)) {
+      // Error encountered for the first time.
+      // After takeError the error is not usable any more in ToDOrErr.
----------------
a_sidorin wrote:
> Is it possible to get this error more than once?
Yes, that can happen in cyclic imports like: ClassTemplateDecl -> TemplatedDecl -> ClassTemplateDecl.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62373/new/

https://reviews.llvm.org/D62373