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

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

I ran the patch on macOS and Linux through check-lldb and there were no 
regressions, so this is LGTM. The libc++ failures should go away when you add 
`libcxx;libcxxabi` to LLVM_ENABLE_PROJECTS (the tests are using libc++).


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

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

In D73675#1858688 , @shafik wrote:

> I forgot to mention this earlier but LLDB is a major user of the 
> `ASTImporter` and so in general it is a good idea to run `check-lldb` as well.


I ran the LLDB tests on Linux, but I get 42 failures:
https://reviews.llvm.org/P8192
If I revert my change, I get the same 42 failures.

I debugged the first test, and it doesn't run because it's trying to use 
libc++.so, but it's not on the library path:

  $ ./a.out
  ./a.out: error while loading shared libraries: libc++.so.1: cannot open 
shared object file: No such file or directory

I can add the `lib` directory inside my checkout to LD_LIBRARY_PATH and it will 
run:

  $ LD_LIBRARY_PATH="$HOME/llvm-project/build/lib" ./a.out
  Test

So, the fix is straightforward, but I think it might take an expert to figure 
out exactly where to put that logic. :)


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

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I forgot to mention this earlier but LLDB is a major user of the `ASTImporter` 
and so in general it is a good idea to run `check-lldb` as well.


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

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

[Reid Kleckner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1091730f5fbb: Avoid many std::tie/tuple instantiations in 
ASTImporter (authored by rnk).

Changed prior to commit:
  https://reviews.llvm.org/D73675?vs=241552=242446#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73675

Files:
  clang/lib/AST/ASTImporter.cpp

Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -186,8 +186,11 @@
   return import(*From);
 }
 
-// Helper for error management in importSeq.
-template  T checkImport(Error , const T ) {
+// Helper for chaining together multiple imports. If an error is detected,
+// subsequent imports will return default constructed nodes, so that failure
+// can be detected with a single conditional branch after a sequence of
+// imports.
+template  T importChecked(Error , const T ) {
   // Don't attempt to import nodes if we hit an error earlier.
   if (Err)
 return T{};
@@ -199,19 +202,6 @@
   return *MaybeVal;
 }
 
-// Import multiple objects with a single function call.
-// This should work for every type for which a variant of `import` exists.
-// The arguments are processed from left to right and import is stopped on
-// first error.
-template 
-Expected> importSeq(const Args &... args) {
-  Error E = Error::success();
-  std::tuple Res{checkImport(E, args)...};
-  if (E)
-return std::move(E);
-  return std::move(Res);
-}
-
 // Wrapper for an overload set.
 template  struct CallOverloadedCreateFun {
   template 
@@ -1150,12 +1140,11 @@
 
 ExpectedType
 ASTNodeImporter::VisitConstantArrayType(const ConstantArrayType *T) {
-  QualType ToElementType;
-  const Expr *ToSizeExpr;
-  if (auto Imp = importSeq(T->getElementType(), T->getSizeExpr()))
-std::tie(ToElementType, ToSizeExpr) = *Imp;
-  else
-return Imp.takeError();
+  Error Err = Error::success();
+  auto ToElementType = importChecked(Err, T->getElementType());
+  auto ToSizeExpr = importChecked(Err, T->getSizeExpr());
+  if (Err)
+return std::move(Err);
 
   return Importer.getToContext().getConstantArrayType(
   ToElementType, T->getSize(), ToSizeExpr, T->getSizeModifier(),
@@ -1175,15 +1164,12 @@
 
 ExpectedType
 ASTNodeImporter::VisitVariableArrayType(const VariableArrayType *T) {
-  QualType ToElementType;
-  Expr *ToSizeExpr;
-  SourceRange ToBracketsRange;
-  if (auto Imp = importSeq(
-  T->getElementType(), T->getSizeExpr(), T->getBracketsRange()))
-std::tie(ToElementType, ToSizeExpr, ToBracketsRange) = *Imp;
-  else
-return Imp.takeError();
-
+  Error Err = Error::success();
+  QualType ToElementType = importChecked(Err, T->getElementType());
+  Expr *ToSizeExpr = importChecked(Err, T->getSizeExpr());
+  SourceRange ToBracketsRange = importChecked(Err, T->getBracketsRange());
+  if (Err)
+return std::move(Err);
   return Importer.getToContext().getVariableArrayType(
   ToElementType, ToSizeExpr, T->getSizeModifier(),
   T->getIndexTypeCVRQualifiers(), ToBracketsRange);
@@ -1191,14 +1177,12 @@
 
 ExpectedType ASTNodeImporter::VisitDependentSizedArrayType(
 const DependentSizedArrayType *T) {
-  QualType ToElementType;
-  Expr *ToSizeExpr;
-  SourceRange ToBracketsRange;
-  if (auto Imp = importSeq(
-  T->getElementType(), T->getSizeExpr(), T->getBracketsRange()))
-std::tie(ToElementType, ToSizeExpr, ToBracketsRange) = *Imp;
-  else
-return Imp.takeError();
+  Error Err = Error::success();
+  QualType ToElementType = importChecked(Err, T->getElementType());
+  Expr *ToSizeExpr = importChecked(Err, T->getSizeExpr());
+  SourceRange ToBracketsRange = importChecked(Err, T->getBracketsRange());
+  if (Err)
+return std::move(Err);
   // SizeExpr may be null if size is not specified directly.
   // For example, 'int a[]'.
 
@@ -1263,26 +1247,24 @@
   }
 
   FunctionProtoType::ExtProtoInfo FromEPI = T->getExtProtoInfo();
+  Error Err = Error::success();
   FunctionProtoType::ExtProtoInfo ToEPI;
-
-  auto Imp = importSeq(
-  FromEPI.ExceptionSpec.NoexceptExpr,
-  FromEPI.ExceptionSpec.SourceDecl,
-  FromEPI.ExceptionSpec.SourceTemplate);
-  if (!Imp)
-return Imp.takeError();
-
   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.NoexceptExpr =
+  importChecked(Err, FromEPI.ExceptionSpec.NoexceptExpr);
+  ToEPI.ExceptionSpec.SourceDecl =
+  importChecked(Err, FromEPI.ExceptionSpec.SourceDecl);
+  ToEPI.ExceptionSpec.SourceTemplate =
+  importChecked(Err, 

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk marked an inline comment as done.
rnk added a comment.

Regarding the `Error Err; ... return std::move(Err);` pattern, I think it needs 
to be written like this for now. At the very least, NRVO is not possible 
because of the Expected conversion, so std::move is not a pessimization.

Thanks for the review!




Comment at: clang/lib/AST/ASTImporter.cpp:1168
+  Error Err = Error::success();
+  QualType ToElementType = T->getElementType();
+  Expr *ToSizeExpr = T->getSizeExpr();

martong wrote:
> shafik wrote:
> > Should this group be using `importChecked` as well?
> Yes, this is a good catch!
Thanks, I fixed this and the one below. I did some regexes to try to find other 
instances based on this pattern, but I didn't find any more. Hopefully I 
haven't introduced new bugs. :)


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

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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:1168
+  Error Err = Error::success();
+  QualType ToElementType = T->getElementType();
+  Expr *ToSizeExpr = T->getSizeExpr();

shafik wrote:
> Should this group be using `importChecked` as well?
Yes, this is a good catch!


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

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:1181
+  Error Err = Error::success();
+  QualType ToElementType = T->getElementType();
+  Expr *ToSizeExpr = T->getSizeExpr();

`importChecked`?


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

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:1168
+  Error Err = Error::success();
+  QualType ToElementType = T->getElementType();
+  Expr *ToSizeExpr = T->getSizeExpr();

Should this group be using `importChecked` as well?


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

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Thank you for this work, it is awesome!

I really like the removal of `importSeq` I feel like the resulting code is more 
clear.


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

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

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

Ok, this looks good to me now. And I really appreciate the work you have done 
here, thanks!


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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon times-circle color=red} Unit tests: fail. 62352 tests passed, 1 failed 
and 839 were skipped.

  failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

{icon times-circle color=red} clang-tidy: fail. clang-tidy found 0 errors and 
147 warnings 
.
 0 of them are added as review comments below (why? 
).

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 


//Pre-merge checks is in beta. Report issue 
.
 Please join beta  or enable 
it for your project 
.//


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

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk updated this revision to Diff 241552.
rnk added a comment.

- Rewrite with importChecked, no importSeq


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73675

Files:
  clang/lib/AST/ASTImporter.cpp

Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -186,8 +186,11 @@
   return import(*From);
 }
 
-// Helper for error management in importSeq.
-template  T checkImport(Error , const T ) {
+// Helper for chaining together multiple imports. If an error is detected,
+// subsequent imports will return default constructed nodes, so that failure
+// can be detected with a single conditional branch after a sequence of
+// imports.
+template  T importChecked(Error , const T ) {
   // Don't attempt to import nodes if we hit an error earlier.
   if (Err)
 return T{};
@@ -199,19 +202,6 @@
   return *MaybeVal;
 }
 
-// Import multiple objects with a single function call.
-// This should work for every type for which a variant of `import` exists.
-// The arguments are processed from left to right and import is stopped on
-// first error.
-template 
-Expected> importSeq(const Args &... args) {
-  Error E = Error::success();
-  std::tuple Res{checkImport(E, args)...};
-  if (E)
-return std::move(E);
-  return std::move(Res);
-}
-
 // Wrapper for an overload set.
 template  struct CallOverloadedCreateFun {
   template 
@@ -1150,12 +1140,11 @@
 
 ExpectedType
 ASTNodeImporter::VisitConstantArrayType(const ConstantArrayType *T) {
-  QualType ToElementType;
-  const Expr *ToSizeExpr;
-  if (auto Imp = importSeq(T->getElementType(), T->getSizeExpr()))
-std::tie(ToElementType, ToSizeExpr) = *Imp;
-  else
-return Imp.takeError();
+  Error Err = Error::success();
+  auto ToElementType = importChecked(Err, T->getElementType());
+  auto ToSizeExpr = importChecked(Err, T->getSizeExpr());
+  if (Err)
+return std::move(Err);
 
   return Importer.getToContext().getConstantArrayType(
   ToElementType, T->getSize(), ToSizeExpr, T->getSizeModifier(),
@@ -1175,15 +1164,12 @@
 
 ExpectedType
 ASTNodeImporter::VisitVariableArrayType(const VariableArrayType *T) {
-  QualType ToElementType;
-  Expr *ToSizeExpr;
-  SourceRange ToBracketsRange;
-  if (auto Imp = importSeq(
-  T->getElementType(), T->getSizeExpr(), T->getBracketsRange()))
-std::tie(ToElementType, ToSizeExpr, ToBracketsRange) = *Imp;
-  else
-return Imp.takeError();
-
+  Error Err = Error::success();
+  QualType ToElementType = T->getElementType();
+  Expr *ToSizeExpr = T->getSizeExpr();
+  SourceRange ToBracketsRange = T->getBracketsRange();
+  if (Err)
+return std::move(Err);
   return Importer.getToContext().getVariableArrayType(
   ToElementType, ToSizeExpr, T->getSizeModifier(),
   T->getIndexTypeCVRQualifiers(), ToBracketsRange);
@@ -1191,14 +1177,12 @@
 
 ExpectedType ASTNodeImporter::VisitDependentSizedArrayType(
 const DependentSizedArrayType *T) {
-  QualType ToElementType;
-  Expr *ToSizeExpr;
-  SourceRange ToBracketsRange;
-  if (auto Imp = importSeq(
-  T->getElementType(), T->getSizeExpr(), T->getBracketsRange()))
-std::tie(ToElementType, ToSizeExpr, ToBracketsRange) = *Imp;
-  else
-return Imp.takeError();
+  Error Err = Error::success();
+  QualType ToElementType = T->getElementType();
+  Expr *ToSizeExpr = T->getSizeExpr();
+  SourceRange ToBracketsRange = T->getBracketsRange();
+  if (Err)
+return std::move(Err);
   // SizeExpr may be null if size is not specified directly.
   // For example, 'int a[]'.
 
@@ -1263,26 +1247,24 @@
   }
 
   FunctionProtoType::ExtProtoInfo FromEPI = T->getExtProtoInfo();
+  Error Err = Error::success();
   FunctionProtoType::ExtProtoInfo ToEPI;
-
-  auto Imp = importSeq(
-  FromEPI.ExceptionSpec.NoexceptExpr,
-  FromEPI.ExceptionSpec.SourceDecl,
-  FromEPI.ExceptionSpec.SourceTemplate);
-  if (!Imp)
-return Imp.takeError();
-
   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.NoexceptExpr =
+  importChecked(Err, FromEPI.ExceptionSpec.NoexceptExpr);
+  ToEPI.ExceptionSpec.SourceDecl =
+  importChecked(Err, FromEPI.ExceptionSpec.SourceDecl);
+  ToEPI.ExceptionSpec.SourceTemplate =
+  importChecked(Err, FromEPI.ExceptionSpec.SourceTemplate);
   ToEPI.ExceptionSpec.Exceptions = ExceptionTypes;
-  std::tie(
-  ToEPI.ExceptionSpec.NoexceptExpr,
-  ToEPI.ExceptionSpec.SourceDecl,
-  ToEPI.ExceptionSpec.SourceTemplate) = *Imp;
+
+  if (Err)
+return std::move(Err);
 
 

[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 , @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 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:1152
+  if (Error E = importSeq(ToElementType, ToSizeExpr))
+return std::move(E);
 

martong wrote:
> Quuxplusone wrote:
> > As the author of [P1155 "More Implicit Move"](https://wg21.link/p1155), I 
> > would expect that you don't need `return std::move(E)` — `return E` should 
> > just as well perform "implicit move" in C++11 and later, assuming that 
> > `llvm::Expected` has a valid constructor from `llvm::Error&&`.
> > 
> > You're not seeing any compiler diagnostic that //suggests// you use 
> > `std::move` here, are you?
> I have some vague and obscure memory about that GCC 4.8 (or 5.2) required 
> explicitly the && cast, otherwise we had a diagnostic (@gamesh411 maybe you 
> help me to recall that?)
Ah, you are absolutely correct: GCC 4.9.4 does not do implicit move in this 
situation, whereas GCC 5.1 and later do do it.
https://godbolt.org/z/YDHtTH
I didn't realize that portability back to GCC 4.9 was still a concern for 
LLVM/Clang. If it is, then please feel free to keep the `std::move` here and 
throughout (but maybe mention it in the commit message for the benefit of 
others who might think as I did).  If portability back to pre-GCC-5 is //not// 
a concern, then I would continue to recommend removing the `std::move`.


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

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a subscriber: gamesh411.
martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:1152
+  if (Error E = importSeq(ToElementType, ToSizeExpr))
+return std::move(E);
 

Quuxplusone wrote:
> As the author of [P1155 "More Implicit Move"](https://wg21.link/p1155), I 
> would expect that you don't need `return std::move(E)` — `return E` should 
> just as well perform "implicit move" in C++11 and later, assuming that 
> `llvm::Expected` has a valid constructor from `llvm::Error&&`.
> 
> You're not seeing any compiler diagnostic that //suggests// you use 
> `std::move` here, are you?
I have some vague and obscure memory about that GCC 4.8 (or 5.2) required 
explicitly the && cast, otherwise we had a diagnostic (@gamesh411 maybe you 
help me to recall that?)


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

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

[Gabor Marton via Phabricator via cfe-commits]

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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 62315 tests passed, 0 failed 
and 838 were skipped.

{icon times-circle color=red} clang-tidy: fail. clang-tidy found 0 errors and 
114 warnings 
.
 0 of them are added as review comments below (why? 
).

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 


//Pre-merge checks is in beta. Report issue 
.
 Please join beta  or enable 
it for your project 
.//


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

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.
Herald added a subscriber: rnkovacs.



Comment at: clang/lib/AST/ASTImporter.cpp:1152
+  if (Error E = importSeq(ToElementType, ToSizeExpr))
+return std::move(E);
 

As the author of [P1155 "More Implicit Move"](https://wg21.link/p1155), I would 
expect that you don't need `return std::move(E)` — `return E` should just as 
well perform "implicit move" in C++11 and later, assuming that 
`llvm::Expected` has a valid constructor from `llvm::Error&&`.

You're not seeing any compiler diagnostic that //suggests// you use `std::move` 
here, are you?


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

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk created this revision.
rnk added reviewers: rsmith, aaron.ballman, a_sidorin.
Herald added a reviewer: martong.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a subscriber: teemperor.
Herald added a project: clang.

Use in-out parameters to avoid making std::tuple values. This change
uses `auto` gratuitously, but I think it actually makes the code more
uniform and more readable. The importer is trying to treat AST nodes in
a uniform way, so I think we can make an exception to the usual coding
standards.

After:

  peak memory: 604.51MB
  real: 0m19.313s
  obj size: 8,404kb

Before:

  peak memory: 954.11MB
  real: 0m26.188s
  obj size: 10,000kb

The speed is not as impressive as I hoped, but the memory use reduction
is impressive, and seems worth it.

Depends on D73667 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73675

Files:
  clang/lib/AST/ASTImporter.cpp

Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -187,29 +187,25 @@
 }
 
 // Helper for error management in importSeq.
-template  T checkImport(Error , const T ) {
-  // Don't attempt to import nodes if we hit an error earlier.
-  if (Err)
-return T{};
-  Expected MaybeVal = import(From);
-  if (!MaybeVal) {
-Err = MaybeVal.takeError();
-return T{};
-  }
-  return *MaybeVal;
+template  void importInPlace(Error , T ) {
+  if (E)
+return;
+  Expected NewNode = import(Node);
+  if (NewNode)
+Node = *NewNode;
+  else
+E = NewNode.takeError();
 }
 
 // Import multiple objects with a single function call.
 // This should work for every type for which a variant of `import` exists.
 // The arguments are processed from left to right and import is stopped on
 // first error.
-template 
-Expected> importSeq(const Args &... args) {
+template  Error importSeq(NodeTys &... Nodes) {
   Error E = Error::success();
-  std::tuple Res{checkImport(E, args)...};
-  if (E)
-return std::move(E);
-  return std::move(Res);
+  char IterNodes[] = {(importInPlace(E, Nodes), '\0')...};
+  (void)IterNodes;
+  return E;
 }
 
 // Wrapper for an overload set.
@@ -1150,12 +1146,10 @@
 
 ExpectedType
 ASTNodeImporter::VisitConstantArrayType(const ConstantArrayType *T) {
-  QualType ToElementType;
-  const Expr *ToSizeExpr;
-  if (auto Imp = importSeq(T->getElementType(), T->getSizeExpr()))
-std::tie(ToElementType, ToSizeExpr) = *Imp;
-  else
-return Imp.takeError();
+  QualType ToElementType = T->getElementType();
+  const Expr *ToSizeExpr = T->getSizeExpr();
+  if (Error E = importSeq(ToElementType, ToSizeExpr))
+return std::move(E);
 
   return Importer.getToContext().getConstantArrayType(
   ToElementType, T->getSize(), ToSizeExpr, T->getSizeModifier(),
@@ -1175,15 +1169,11 @@
 
 ExpectedType
 ASTNodeImporter::VisitVariableArrayType(const VariableArrayType *T) {
-  QualType ToElementType;
-  Expr *ToSizeExpr;
-  SourceRange ToBracketsRange;
-  if (auto Imp = importSeq(
-  T->getElementType(), T->getSizeExpr(), T->getBracketsRange()))
-std::tie(ToElementType, ToSizeExpr, ToBracketsRange) = *Imp;
-  else
-return Imp.takeError();
-
+  QualType ToElementType = T->getElementType();
+  Expr *ToSizeExpr = T->getSizeExpr();
+  SourceRange ToBracketsRange = T->getBracketsRange();
+  if (Error E = importSeq(ToElementType, ToSizeExpr, ToBracketsRange))
+return std::move(E);
   return Importer.getToContext().getVariableArrayType(
   ToElementType, ToSizeExpr, T->getSizeModifier(),
   T->getIndexTypeCVRQualifiers(), ToBracketsRange);
@@ -1191,14 +1181,11 @@
 
 ExpectedType ASTNodeImporter::VisitDependentSizedArrayType(
 const DependentSizedArrayType *T) {
-  QualType ToElementType;
-  Expr *ToSizeExpr;
-  SourceRange ToBracketsRange;
-  if (auto Imp = importSeq(
-  T->getElementType(), T->getSizeExpr(), T->getBracketsRange()))
-std::tie(ToElementType, ToSizeExpr, ToBracketsRange) = *Imp;
-  else
-return Imp.takeError();
+  QualType ToElementType = T->getElementType();
+  Expr *ToSizeExpr = T->getSizeExpr();
+  SourceRange ToBracketsRange = T->getBracketsRange();
+  if (Error E = importSeq(ToElementType, ToSizeExpr, ToBracketsRange))
+return std::move(E);
   // SizeExpr may be null if size is not specified directly.
   // For example, 'int a[]'.
 
@@ -1264,25 +1251,21 @@
 
   FunctionProtoType::ExtProtoInfo FromEPI = T->getExtProtoInfo();
   FunctionProtoType::ExtProtoInfo ToEPI;
-
-  auto Imp = importSeq(
-  FromEPI.ExceptionSpec.NoexceptExpr,
-  FromEPI.ExceptionSpec.SourceDecl,
-  FromEPI.ExceptionSpec.SourceTemplate);
-  if (!Imp)
-return Imp.takeError();
-
   ToEPI.ExtInfo = FromEPI.ExtInfo;
   ToEPI.Variadic =