martong added a comment.
Thanks for this patch too!
However, I have a minor concern with this new approach:
If `importSeq` fails then a `ToSomething` still holds a value from the "From"
context. This could cause problems if it is used later in creating the new AST
node. Normally, this should not happen because we do check the return value of
`importSeq`. But as the AST increases, new properties are added to a node,
let's say `ToSomething2` and when we forget (by mistake) to add this to
`importSeq` then we end up having a node that has a reference to something in
the "From" context:
auto ToSomething = From->getToSomething();
auto ToSomething2 = From->getToSomething2();
if (Error E = ImportSeq(ToSomething))
return std::move(E);
createNewNodeInToCtx(ToSomething, ToSomething2);
In contrast to the original behavior where we would end up with an
uninitialized value (pointer) which seems to be easier to recognize when
someone uses the merged AST.
(I am not sure how could we force `ToSomething2` into `importSeq`, perhaps a
clang-tidy checker could do that.)
================
Comment at: clang/lib/AST/ASTImporter.cpp:1265
+
+ if (Error E = importSeq(ToEPI.ExceptionSpec.NoexceptExpr,
+ ToEPI.ExceptionSpec.SourceDecl,
----------------
My concern here is that if `importSeq` fails here then
`ToEPI.ExceptionSpec.NoexceptExpr` still holds a value from the "From" context
and that can be a (fatal) problem later. The import should happen right after
the initialization of these variables.
So, we should have something like this:
```
ToEPI.ExceptionSpec.NoexceptExpr = FromEPI.ExceptionSpec.NoexceptExpr;
ToEPI.ExceptionSpec.SourceDecl = FromEPI.ExceptionSpec.SourceDecl;
ToEPI.ExceptionSpec.SourceTemplate = FromEPI.ExceptionSpec.SourceTemplate;
if (Error E =
importSeq(ToEPI.ExceptionSpec.NoexceptExpr,ToEPI.ExceptionSpec.SourceTemplate))
return std::move(E);
ToEPI.ExtInfo = FromEPI.ExtInfo;
ToEPI.Variadic = FromEPI.Variadic;
ToEPI.HasTrailingReturn = FromEPI.HasTrailingReturn;
ToEPI.TypeQuals = FromEPI.TypeQuals;
ToEPI.RefQualifier = FromEPI.RefQualifier;
ToEPI.ExceptionSpec.Type = FromEPI.ExceptionSpec.Type;
ToEPI.ExceptionSpec.Exceptions = ExceptionTypes;
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73675/new/
https://reviews.llvm.org/D73675
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
