This is an archive of the discontinued LLVM Phabricator instance.

[coroutines] Add DependentCoawaitExpr and fix re-building CoroutineBodyStmt.
ClosedPublic

Authored by EricWF on Oct 27 2016, 6:22 PM.

Details

Summary

The changes contained in this patch are:

  1. Defines a new AST node CoawaitDependentExpr for representing co_await expressions while the promise type is still dependent.
  2. Correctly detect and transform the 'co_await' operand to p.await_transform(<expr>) when possible.
  3. Change the initial/final suspend points to build during the initial parse, so they have the correct operator co_await lookup results.
  4. Fix transformation of the CoroutineBodyStmt so that it doesn't re-build the final/initial suspends.

@rsmith: This change is a little big, but it's not trivial for me to split it up. Please let me know if you would prefer this submitted as multiple patches.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 76155.Oct 27 2016, 6:22 PM
EricWF retitled this revision from to [coroutines] Add CoawaitDependentExpr AST node and use it to properly build await_transform..
EricWF updated this object.
EricWF added reviewers: rsmith, GorNishanov.
EricWF added subscribers: cfe-commits, rsmith.
EricWF updated this revision to Diff 76157.Oct 27 2016, 6:27 PM
  • Remove old FIXME
  • Correct doc for CoawaitDependentExpr.
EricWF added inline comments.Oct 27 2016, 7:13 PM
lib/AST/ExprClassification.cpp
193

@rsmith: What's the correct way to classify this type?

lib/Sema/SemaCoroutine.cpp
36

The changes to this function are all unrelated cleanup/improvements.

EricWF updated this revision to Diff 76167.Oct 27 2016, 11:37 PM
  • Suppress diagnostics caused during the initial name lookup for await_transform, return_value and return_void since these diagnostics will get re-emitted if we actually build the calls.
  • add more tests.
ABataev added inline comments.
include/clang/AST/ExprCXX.h
4265

const UnresolvedSetImpl &

4266–4267

Add comments with the name of params on true args

4277

s/static_cast<Expr *>/cast<Expr>/g

4279

const UnresolvedSetImpl &

include/clang/Sema/Sema.h
8040

const UnresolvedSetImpl &

lib/Sema/SemaCoroutine.cpp
82

auto *RD

240–241

Maybe it is better to add an argument UnresolvedSetImpl &OpCandidates?

248

const UnresolvedSetImpl &Functions

288

Why do you need MutableArrayRef<Epr *>, when ArrayRef<Expr *> is enough? Also buildMemberCall must be changed

411

s/BuildCoawaitDependentExpr/buildCoawaitDependentExpr/g

412

const UnresolvedSetImpl &

434

auto *RD

lib/Sema/TreeTransform.h
1327

const UnresolvedSetImpl &

EricWF marked 10 inline comments as done.Oct 31 2016, 5:17 AM
EricWF added inline comments.
lib/Sema/SemaCoroutine.cpp
240–241

This seems like the best place to specify the concrete type now that everything else uses UnresolvedSetImpl.

288

Sema::ActOnCallExpr requires the MutableArrayRef, so I don't see how this can be changed.

411

Why? Every similar function uses a leading capital.

EricWF updated this revision to Diff 76386.Oct 31 2016, 5:20 AM

Mark some review comments as complete.

ABataev added inline comments.Oct 31 2016, 5:35 AM
lib/Sema/SemaCoroutine.cpp
240–241

Does really matter that it is UnresolvedSet<16>, but not UnresolvedSet<8> or UnresolvedSet<4>? If not, you should not use UnresolvedSet<16> as the param type or return type

288

Ok, then maybe it is better to use MultiExprArg rather than MutableArrayRef<Expr *> which is looking a bit confusing

EricWF added inline comments.Oct 31 2016, 5:55 AM
lib/Sema/SemaCoroutine.cpp
240–241

What matters is that UnresolvedSetImpl is an abstract interface and this function returns by value. At some point we have to choose an N for UnresolvedSet<N> and this is the correct place to do it.

EricWF updated this revision to Diff 76390.Oct 31 2016, 6:05 AM
EricWF marked an inline comment as done.
  • Use MultiExprArg in place of MutableArrayRef<Expr *>.

Thanks for looking at this @ABataev!

EricWF planned changes to this revision.Oct 31 2016, 6:59 AM

I think there's a better way to do this. The implicitly created initial and final suspend also require the overload set for operator co_await, so I think a better approach would be to store the lookup results inside FunctionDecl.

rsmith added inline comments.Nov 1 2016, 5:47 PM
include/clang/AST/ExprCXX.h
4256

Rename -> DependentCoawaitExpr, to match our other Dependent*Expr classes.

4259

Would it make sense to store this as an UnresolvedLookupExpr* instead?

lib/AST/ExprClassification.cpp
193

See above:

// Unresolved lookups and uncorrected typos get classified as lvalues.
// FIXME: Is this wise? Should they get their own kind?

It shouldn't ever matter what value category we give to a type-dependent expression.

lib/Sema/TreeTransform.h
6707–6708

This should ideally call into the same function we use for the non-dependent case.

EricWF updated this revision to Diff 76952.Nov 4 2016, 4:47 PM
EricWF retitled this revision from [coroutines] Add CoawaitDependentExpr AST node and use it to properly build await_transform. to [coroutines] Add DependentCoawaitExpr and fix re-building CoroutineBodyStmt..
EricWF updated this object.
EricWF marked an inline comment as done.
  • Address review comments about DependentCoawaitExpr and using UnresolvedLookupExpr.
  • Fix building of the initial/final coroutine suspends points.
  • Fix transformation of CoroutineBodyStmt so that it transforms the final/initial suspend points instead of rebuilding them fully.

@rsmith: This change is a little big, but it's not trivial for me to split it up. Please let me know if you would prefer this submitted as multiple patches.

EricWF updated this revision to Diff 76953.Nov 4 2016, 4:50 PM
  • Address review comments about DependentCoawaitExpr and using UnresolvedLookupExpr.
  • Fix building of the initial/final coroutine suspends points.
  • Fix transformation of CoroutineBodyStmt so that it transforms the final/initial suspend points instead of rebuilding them fully.

@rsmith: This change is a little big, but it's not trivial for me to split it up. Please let me know if you would prefer this submitted as multiple patches.

@rsmith Ping. Would you like me to split this up into smaller change sets?

rsmith added inline comments.Jan 13 2017, 11:58 AM
include/clang/AST/ExprCXX.h
4237

I would go with just isImplicit, to match other similar uses throughout the AST. Also maybe sink this into the Stmt bitfields to make this class 8 bytes smaller

lib/AST/ItaniumMangle.cpp
3302–3303

I don't think "should no longer exist" is true. If co_await can appear in a function signature at all, it can appear with a dependent operand. This should be mangled the same as a non-dependent co_await expression.

lib/Sema/SemaCoroutine.cpp
36

Just go ahead and commit these separately then.

99

buildElaboratedType would be a better name for what this does. I also wonder if this is really necessary, or whether we can just use %q0 in the diagnostic format to request a fully-qualified type name.

112–116

We used to return the ElaboratedType and don't do so any more.

409–414

This seems like an odd naming choice. I'd expect BuildDependentCoawaitExpr to only deal with the case where the expression is dependent (and to never be called otherwise), and BuildCoawaitExpr to handle both the case where the expression is dependent and the case where it is non-dependent. Maybe the other function should be called BuildResolvedCoawaitExpr or similar.

lib/Sema/SemaDecl.cpp
11386

Stray change.

lib/Sema/TreeTransform.h
6687–6689

Please use a term other than "invalid" here. We generally use "invalid" to mean "an error has been diagnosed, use best-effort recovery".

6802

You need to transform the UnresolvedLookupExpr here too. (Example: we might have a function-local declaration of operator co_await that needs to be transformed into the version in the instantiated function.)

EricWF updated this revision to Diff 88300.Feb 13 2017, 9:14 PM
EricWF marked 6 inline comments as done.

Merge with upstream and address almost all inline comments.

The only comment left te address is @rsmith's comment about renaming BuildDependentCoawaitExpr.

EricWF updated this revision to Diff 88304.Feb 13 2017, 9:46 PM

Re-add tests that got lost in the merge.

EricWF updated this revision to Diff 88319.Feb 14 2017, 12:05 AM
  • More cleanup
  • Rename BuildCoawaitExpr -> BuildResolvedCoawaitExpr and BuildDependentCoawaitExpr -> BuildUnresolvedCoawaitExpr.
EricWF updated this revision to Diff 88325.Feb 14 2017, 12:40 AM

Add more tests.

rsmith accepted this revision.Mar 6 2017, 2:18 PM
rsmith added inline comments.
include/clang/Sema/ScopeInfo.h
138–140

Is the comment here correct? It seems a slightly odd match for the member name.

lib/Sema/SemaCoroutine.cpp
36

Please commit these changes separately if they're cleanups unrelated to the main purpose of the patch.

lib/Sema/TreeTransform.h
6974–6975

Remove this :) (This is already checked by the cast below.)

This revision is now accepted and ready to land.Mar 6 2017, 2:18 PM
EricWF updated this revision to Diff 90752.Mar 6 2017, 3:11 PM
EricWF marked an inline comment as done.
  • Address @rsmith's final review comments.
EricWF added inline comments.Mar 6 2017, 3:13 PM
include/clang/Sema/ScopeInfo.h
138–140

The comment should probably say "True only when this function has not already built, or attempted to build, the initial and final coroutine suspend points".

lib/Sema/SemaCoroutine.cpp
36

Ack.

99

We can't use "%q0" in the diagnostic passed to RequireCompleteType, so we have to build up the elaborated QualType instead.

112–116

I don't think that's true. We only build the ElaboratedType before issuing a diagnostic and returning QualType() to signal failure.

lib/Sema/TreeTransform.h
6802

Ack. Fixed.

Do you have an example that doesn't use a function-local definition? Since operator co_await cannot be defined locally.

rsmith added inline comments.Mar 6 2017, 3:48 PM
lib/Sema/TreeTransform.h
6802

It can't be defined locally, but it can be *declared* locally. Example:

template<typename T> future<void> f(T t) {
  future<void> operator co_await(T);
  co_await t;
}
struct X {};
auto x = f(X{});

... which would require that future<void> operator co_await(X) is defined somewhere in the program.

EricWF closed this revision.Mar 6 2017, 3:50 PM