Page MenuHomePhabricator

[Coroutines] Do not evaluate InitListExpr of a co_return
ClosedPublic

Authored by junparser on Mar 12 2020, 11:22 PM.

Details

Summary

This simple patch fixes the assertion which happens in ScalarExprEmitter::VisitInitListExpr when we try to compute the expression and ignore the result.

the test case here is reduced by c-reduce.

TestPlan: check-clang

Diff Detail

Event Timeline

junparser created this revision.Mar 12 2020, 11:22 PM
modocache accepted this revision.Mar 13 2020, 5:15 AM

Awesome, thank you! This is an issue I've been running into as well in an internal codebase, I'll check whether this patch addresses it.

My suspicion is that the test case could be reduced further, but the implementation itself seems like a straightforward improvement to me.

clang/lib/CodeGen/CGCoroutine.cpp
278–280

Nice. And it looks like such a check also exists in SemaCoroutine.cpp, added in D25296.

280

nit-pick: I slightly prefer "initializer list" over "initlist", but it's your call.

clang/test/CodeGenCoroutines/coro-return-voidtype-initlist.cpp
87

I'm surprised this is the smallest creduce is capable of reducing this test case. This seems like the most important line, since it co_returns an initializer list. My suspicion is that this test case could be reduced further. Certainly the coroutine_traits and definition of the awaitable type l::m would need to remain, but do the functions below this line need to exist?

I also feel creduce's typedefs, such as template <typename = void> using i = std::experimental::coroutine_handle<>, make this example more confusing. They reduce character count, but std::experimental::coroutine_handle is a common type in C++ coroutines code, whereas i is not necessarily.

This revision is now accepted and ready to land.Mar 13 2020, 5:15 AM
junparser marked an inline comment as done.Mar 15 2020, 6:43 PM

@modocache thanks.

clang/test/CodeGenCoroutines/coro-return-voidtype-initlist.cpp
87

I try to remove the functions below which makes the case can not address this issue. So for this case, I'll try to simplify it as far as i can.

junparser updated this revision to Diff 250463.Mar 15 2020, 9:41 PM

simplify the testcase

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2020, 9:44 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript