[PATCH] D122525: [clang][ASTImporter] Fix an import error handling related bug.
Balázs Kéri via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 6 07:41:32 PDT 2022
balazske added inline comments.
================
Comment at: clang/lib/AST/ASTImporter.cpp:8792-8794
+ if (!isa<TagDecl>(FromDi))
+ if (auto *FromDiDC = dyn_cast<DeclContext>(FromDi))
+ if (FromDiDC->containsDecl(PrevFromDi))
----------------
martong wrote:
> 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
> }
>
> };
> ```
The `AccumulateChildErrors` value looks the only common thing. It can mean: Accumulate errors (or not) for declarations that are in child-relationship with the parent. The value is only dependent on the parent, true if it is a `TagDecl`. This value can be extracted into a function that is used in `importDeclContext` and at `Import(Decl *)`. At `importDeclContext` we should accumulate or discard errors for every child node. But not for possible other nodes that are imported but are not a child node (probably no such exists now but theoretically it is possible, and when using the error handling strategy for non-namespace-like nodes).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122525/new/
https://reviews.llvm.org/D122525
More information about the cfe-commits
mailing list