[PATCH] D30809: [coroutines] Add codegen for await and yield expressions

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/CodeGen/CGCoroutine.cpp:32
 
 struct CGCoroData {
+  AwaitKind CurrentAwaitKind = AwaitKind::Init;
----------------
GorNishanov wrote:
> majnemer wrote:
> > Shouldn't this struct be in an anonymous namespace?
> It is forward declared in CodeGenFunction.h and CodeGenFunction class holds a unique_ptr to CGCoroData as a member.
> Here is the snippet from CodeGenFunction.h:
> 
> ```
>   // Holds coroutine data if the current function is a coroutine. We use a
>   // wrapper to manage its lifetime, so that we don't have to define CGCoroData
>   // in this header.
>   struct CGCoroInfo {
>     std::unique_ptr<CGCoroData> Data;
>     CGCoroInfo();
>     ~CGCoroInfo();
>   };
>   CGCoroInfo CurCoro;
> ```
If it's already forward-declared, you should check here that you're matching it by doing:

  struct CodeGen::CGCoroData {
    ...
  };

instead of making namespace declarations.


================
Comment at: lib/CodeGen/CGCoroutine.cpp:42
+  unsigned YieldNum = 0;
   unsigned CoreturnCount = 0;
 
----------------
Comments about what these mean would be useful.


================
Comment at: lib/CodeGen/CGCoroutine.cpp:85
+  unsigned No = 0;
+  StringRef AwaitKindStr = 0;
+  switch (Kind) {
----------------
majnemer wrote:
> I'd just let the default constructor do its thing.
Agreed.  Assigning 0 to a StringRef reads very wrong.


================
Comment at: lib/CodeGen/CGCoroutine.cpp:203
+    break;
+  }
+  return nullptr;
----------------
This function should return an RValue and take an AggValueSlot, just like every other generically-typed expression emitter.  That way, you can just use EmitAnyExpr here instead of inlining two of three cases.

And you should implement it in CGExprComplex, of course, and add that as a test case.


https://reviews.llvm.org/D30809