[PATCH] D122525: [clang][ASTImporter] Fix an import error handling related bug.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Nice catch!



================
Comment at: clang/lib/AST/ASTImporter.cpp:8789-8791
+        // The import path contains child nodes first.
+        // (Parent-child relationship is used here in sense of import
+        // dependency.)
----------------
I think, this comment would be better to describe the declaration of `PrevFromDi`.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8792-8794
+        if (!isa<TagDecl>(FromDi))
+          if (auto *FromDiDC = dyn_cast<DeclContext>(FromDi))
+            if (FromDiDC->containsDecl(PrevFromDi))
----------------
We should elevate this logic and the one in `ImportDeclContext` that uses `consumeError` into a common abstraction. I.e. making them closer to each other in a class.
Something like (draft, did not test it, might not compile):
```
class ErrorHandlingStrategy {
  static bool accumulateChildErrors(DeclContext*);
  static bool accumulateChildErrors(Decl *FromDi, Decl* Parent); // Use this one when traversing through the import path!
public:
  static Error handleDeclContextError(Error Err, DeclContext* FromDC) {
    Error ChildErrors = Error::success();
            if (Err && accumulateChildErrors(FromDC))
              ChildErrors =  joinErrors(std::move(ChildErrors), std::move(Err));
            else
              consumeError(std::move(Err));
     return ChildErrors
  }
  
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122525