This is an archive of the discontinued LLVM Phabricator instance.

[coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.
ClosedPublic

Authored by GorNishanov on Jan 17 2017, 4:38 PM.
Tokens
"Mountain of Wealth" token, awarded by GorNishanov.

Details

Summary

Sema::CheckCompletedCoroutineBody was growing unwieldy with building all of the substatements. Also, constructors for CoroutineBodyStmt had way too many parameters.

Instead, CoroutineBodyStmt now defines CtorArgs structure with all of the required construction parameters.
CheckCompleteCoroutineBody delegates construction of individual substatements to short functions one per each substatement.

Also, added a drive-by fix of initializing CoroutinePromise to nullptr in ScopeInfo.h.
And addressed the FIXME that wanted to tail allocate extra room at the end of the CoroutineBodyStmt to hold parameter move expressions. (The comment was longer that the code that implemented tail allocation).

Diff Detail

Repository
rL LLVM

Event Timeline

GorNishanov created this revision.Jan 17 2017, 4:38 PM
rsmith edited edge metadata.Jan 23 2017, 11:38 AM

Generally looks good, but we have a better way of modeling types with a trailing variable-length array that you should use.

include/clang/AST/StmtCXX.h
299 ↗(On Diff #84775)

Please use llvm::TrailingObjects to store the trailing variable-length SubStmts array.

lib/Sema/SemaCoroutine.cpp
714–722 ↗(On Diff #84775)

Reindent comments.

No changes. Merge with top of the tree (to simplify comparing with the updated version that is coming up in a second).

Feedback implemented!

GorNishanov marked 2 inline comments as done.Jan 23 2017, 4:56 PM

Looks even better now!

LGTM? Pretty please :)

Gentle and melodic ping.

EricWF requested changes to this revision.Feb 10 2017, 2:16 PM

This currently segfaults on my machine. Here is the full output of running SemaCXX/coroutines.cpp.
https://gist.github.com/EricWF/81dc332e21c3e5c6bdc024cda87b846f

I'm not sure exactly what's going on, but it should be fixed before committing.

This revision now requires changes to proceed.Feb 10 2017, 2:16 PM

This LGTM after applying the fixes.

lib/Sema/SemaCoroutine.cpp
719 ↗(On Diff #85490)

I figured out what's going on. PromiseRecordDecl doesn't get initialized to null when IsPromiseDependentType is false. Initializing PromiseRecordDecl fixes the problem.

GorNishanov edited edge metadata.

Initialized member variable to zero.

Initialized PromiseRecordDecl to nullptr

GorNishanov marked an inline comment as done.Feb 12 2017, 7:54 PM

Preparing to land. Thank you very much for review!

lib/Sema/SemaCoroutine.cpp
719 ↗(On Diff #85490)

Good catch!

This revision was automatically updated to reflect the committed changes.
GorNishanov marked an inline comment as done.