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

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 CHANG

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

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 failu

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

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

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

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&id=242446#toc Repository: rG LLVM Github Mo

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

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

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

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 w

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

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 CHANGE

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

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

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

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 ___

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

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/D7367

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

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 14

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

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

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

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 `ToS

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

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

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

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 Mov

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

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

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

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

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

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

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

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::t