This is an archive of the discontinued LLVM Phabricator instance.

[coroutines] Add allocation and deallocation substatements.
ClosedPublic

Authored by GorNishanov on Oct 21 2016, 12:51 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

GorNishanov retitled this revision from to [coroutines] Add allocation and deallocation substatements..
GorNishanov updated this object.
GorNishanov added a reviewer: rsmith.
GorNishanov added subscribers: llvm-commits, EricWF.
ABataev added inline comments.
lib/Sema/SemaCoroutine.cpp
165 ↗(On Diff #75464)

id does not fllow LLVM coding rules. Must be Id

169 ↗(On Diff #75464)

true argument must have adjacent comment with the name of the parameter, like /*Param=*/true

171 ↗(On Diff #75464)

auto *BuiltInDecl

174–175 ↗(On Diff #75464)

I believe you can drop the last nullptr arg, it is defaulted. And I think it's better to make it VK_LValue

409 ↗(On Diff #75464)

auto *PointeeRD

472–473 ↗(On Diff #75464)

The last nullptr can be dropped

481 ↗(On Diff #75464)

OpDeleteQualType

494 ↗(On Diff #75464)

const auto *OpDeleteType

496–498 ↗(On Diff #75464)

No need for braces in one-line substatement

501 ↗(On Diff #75464)

Drop last nullptr

Thank you for review! Applied suggested changes.

GorNishanov marked 10 inline comments as done.

Formatting changes (NFC)

rsmith accepted this revision.Oct 27 2016, 12:22 AM
rsmith edited edge metadata.

LGTM with minor changes.

lib/CodeGen/CGCoroutine.cpp
71–72 ↗(On Diff #75946)

Can this really happen? The CoroutineBodyStmt should be the first thing we emitted into a new function, so it seems like we shouldn't have pre-existing coro data yet.

lib/Sema/SemaCoroutine.cpp
422 ↗(On Diff #75946)

This should be outside the if.

This revision is now accepted and ready to land.Oct 27 2016, 12:22 AM
GorNishanov edited edge metadata.

Thank you for review! Applied suggested changes and preparing to land.

GorNishanov marked 2 inline comments as done.Oct 27 2016, 6:47 AM
GorNishanov added inline comments.
lib/CodeGen/CGCoroutine.cpp
71–72 ↗(On Diff #75946)

Yep. You are right! I took it out and made createCoroData return void, since nobody cares about its result anymore.

GorNishanov marked an inline comment as done.

Merged prior to landing. Incoming ...

This revision was automatically updated to reflect the committed changes.