ping～

Rebase

Thanks for the review! @ldionne

ping~

Thank you for your review @aaron.ballman @rsmith , I will be happy to continue cook this patch once we reach a consensus.

Address Aaron's comments.

Addres the comments that we talked in D154784.

Thanks for your review @ldionne

Address comments

friendly ping~

Thanks for your review! @Mordante @philnik

I have reverted this patch and reopen.

In D158749#4619918, @philnik wrote:

Shouldn't this be possible to actually test? If not, please add a comment why this change was done.

Many thanks for your review! @Mordante

Address comments.

Addres comments.

I'm so sorry that I have no idea how to test stream size overflow in this change now.

In D154116#4612494, @frederick-vs-ja wrote:

In D154116#4490038, @yronglin wrote:

@Mordante Can we propose to use __union_.__val_, std::as_const(__union_.__val_), std::move(__union_.__val_) and std::move(std::as_const(__union_.__val_)) instead of **this ?

std::as_const is unnecessary because the const on the member functions already makes __union_.__val_ a const lvalue.

I think it's better to use the member access expressions instead of **this, because the latter can be ADL-hijacked. (However, this means the current standard wording is less than ideal and an LWG issue may be needed, see also LWG3969).

Address comments

ping~ @hubert.reinterpretcast @rsmith

Address comments

@cor3ntin Thanks for your review!

Do not spass MaterializePRValueInDiscardStatement in PushExpressionEvaluationContext.

Fix ci

Fix ci failure, and introduce an variable in ExpressionEvaluationContextRecord to rewrite default argument.

Thanks a lot for your review! @Mordante

Addres comment.

In D155701#4592617, @Mordante wrote:

In D155701#4592515, @yronglin wrote:

ping~

I've been quite busy. I'll hope to have time to look at this patch and D154116 later this week.

ping~

friendly ping~

In D153701#4563919, @yronglin wrote:

Sorry for the late reply. I tried to investigate the memory impact of creating these additional materializations by build the whole llvm-project and compare the number of MaterializedTemporaryExpr created during parsing.

Steps:

1. Add a public member uint64_t NumMetarilizedTemporaryExpr = 0; in ASTContext .
2. Increment the value of NumMetarilizedTemporaryExpr in Sema::CreateMaterializeTemporaryExpr.
3. Write the NumMetarilizedTemporaryExpr into a text file when ASTContext destruction, each translation unit will append a line to the file to record the value of NumMetarilizedTemporaryExpr .
4. Build the entire llvm-project separately using the compiler that creates addational materializations and the compiler that doesn't.
5. Sum the numbers produced by each translation unit.

The result is:

Item	Count
Addational Materialized Temporarys Total	50655585
Clang Trunk Total	18346347
Diff	32309238

The more detail result in https://yronglin.github.io

The gap between these two numbers is very large. So I'think we can create additional materializations only within for-range initializers. I'm not sure if I can find a way to only create materializes for temporaries that need to have an extended lifetime, WDYT?

@cor3ntin I have reused EnsureImmediateInvocationInDefaultArgs to rewrite CXXDefaultArgExpr, does this is a correct approach?

Add test to check generated LLVM IR, and fix crash.

Should we dump the real default argument AST but not only CXXDefaultArgExpr if CXXDefaultArgExpr has been rewritted?

FIXME: Need add test in clang/test/CXX/special/class.temporary/p6.cpp to check generated LLVM IR.

Handle default argument and dependent context.

Only create addational metarialized temporary in for-range-init.
FIXME: Need to handle function default argument, and add more test to make sure the generated LLVM IR is correct.

In D153701#4575056, @hubert.reinterpretcast wrote:

In D153701#4563919, @yronglin wrote:

The gap between these two numbers is very large. So I'think we can create additional materializations only within for-range initializers. I'm not sure if I can find a way to only create materializes for temporaries that need to have an extended lifetime, WDYT?

@rsmith asked for something slightly different (but I am not sure how to collect that info): He wanted the AST size delta.

For what we have, I think the detailed results table would be more useful if we add some size metric for the files and the increase expressed as a percentage. Perhaps sorting by file size would help contextualize the significance of the percentage change.

Thanks for your review! @hokein @tbaeder

Address comments.

Sorry for the late reply. I tried to investigate the memory impact of creating these additional materializations by build the whole llvm-project and compare the number of MaterializedTemporaryExpr created during parsing.

Address comment, use _If to get closer to the standard wording.

@Mordante ping~

Thanks a lot for your review! @Mordante

Address comments.

Fix CI

In D155701#4519553, @Mordante wrote:

Does this have test for all changes paragraphs in the LWG issue?

Address comments.

This is a very early implement, please give me some time to add and fix tests.

Thanks for your review and sorry for the very late reply!

Thanks for your comments and sorry for the very late reply, I have been investigating how to achieve it these days.

Try implement

Rebase

Seems std::optional does not have this issue https://godbolt.org/z/96T5MT695

In D141699#4513484, @var-const wrote:

In D141699#4513287, @yronglin wrote:

NFC, fix ci, use _LIBCPP_ENABLE_DEBUG_MODE instead of _LIBCPP_ENABLE_ASSERTIONS

Rather than explicitly enabling debug mode (which will fail in the hardened mode test), please add // UNSUPPORTED: !libcpp-has-hardened-mode && !libcpp-has-debug-mode

Address comments

NFC, fix ci, use _LIBCPP_ENABLE_DEBUG_MODE instead of _LIBCPP_ENABLE_ASSERTIONS

Waiting for CI green

In D141699#4510812, @Mordante wrote:

I mainly skimmed over it. I trust @philnik and @var-const to have done a deeper review; they are more familiar with the ranges details.

NFC, Use + instead of std::ranges::next in tests.