[PATCH] D73675: Avoid many std::tie/tuple instantiations in ASTImporter

[Reid Kleckner via Phabricator via cfe-commits]

rnk planned changes to this revision.
rnk marked an inline comment as done.
rnk added a comment.

In D73675#1849182 <https://reviews.llvm.org/D73675#1849182>, @martong wrote:

> 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.)


It occurs to me that we don't really need importSeq. The only thing it's doing for us is to help tidy up the error checking. All this code really needs is an accurate list of all the AST nodes to import from one AST to another. We could use a code pattern like this instead:

  Error E = Error::success();
  auto ToType = checkedImport(E, D->getType());
  auto ToLoc = checkedImport(E, D->getLoc());
  auto ToTSI = checkedImport(E, D->getTypeSourceInfo());
  ...
  if (E)
    return std::move(E);

`checkedImport` would have similar behavior to the current helpers:

- if there was already an error, do nothing, return a default constructed node (null, sloc 0, etc)
- if importing produces an error, store it in the error, return a default constructed node
- else, return the node

I think this would address your concern that the `To` nodes are always in the new ASTContext.

I'll see if I can redo this in that direction. I should be able to rewrite all of the Expr imports, which are very uniform, automatically...



================
Comment at: clang/lib/AST/ASTImporter.cpp:206
       Error E = Error::success();
-      std::tuple<Args...> Res{checkImport(E, args)...};
-      if (E)
-        return std::move(E);
-      return std::move(Res);
+      char IterNodes[] = {(importInPlace(E, Nodes), '\0')...};
+      (void)IterNodes;
----------------
I should add that I'm a novice with variadic templates. If anyone else has suggestions for a better way to iteratively call `importInPlace` on each argument, left to right, I'm open to it. I found this initializer list trick on stack overflow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73675