martong 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))
----------------
balazske wrote:
> 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).
My point is to try to encapsulate the error handling logic as much as possible.
It is not just about `AccumulateChildErrors`. The current change scatters the
error handling logic into seemingly distant code parts. The
`ErrorHandlingStrategy` class could serve this encapsulation, because that's
what classes do.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122525/new/
https://reviews.llvm.org/D122525
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
