sammccall added a subscriber: rsmith.
sammccall added a comment.
This generally looks really good, I'm worried about the separate transformation
in the template case though.
================
Comment at: clang/include/clang/AST/ExprCXX.h:4731
+ Expr *getOperand() const {
+ return static_cast<Expr *>(SubExprs[SubExpr::Operand]);
----------------
I think a comment here like `// The syntactic operand written in the code` or
so would help clarify the distinction between this and the
common/ready/suspend/resume family.
I'm just thinking of all the times working on tooling when you're trying to
understand how to handle every node type without deep-diving into the semantics
of each one :-)
Also maybe group getOperand(), getKeywordLoc(), getBeginLoc(), getEndLoc()
together?
================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:737
return StmtError();
- Suspend = BuildResolvedCoawaitExpr(Loc, Suspend.get(),
+ Suspend = BuildResolvedCoawaitExpr(Loc, Suspend.get(), Suspend.get(),
/*IsImplicit*/ true);
----------------
I'm not sure Suspend.get() here is the most appropriate value for Operand.
It's a little abstract, since we're talking about the syntactic operand of an
implicit call :-)
But for consistency with the explicit case, I think the operand should be the
result of `initial_suspend()` etc, *before* we call `operator co_await`.
i.e. the value of Suspend.get() a couple of lines above.
================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:838
+ Expr *Operand = E;
+
----------------
This has become confusing I think. The old code was was changing the meaning of
the `E`, because we didn't need the original anymore. Rather than introduce a
new variable for the original meaning, I'd prefer to introduce one for the
changed meaning:
```
// line 848
auto *Transformed = E;
if (lookupMember(...)) {
...
Transformed = R.get();
}
buildOperatorCoAwaitCall(..., Transformed);
```
(if you also want to rename E -> Operand, that makes sense to me too!)
================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:858
}
ExprResult Awaitable = buildOperatorCoawaitCall(*this, Loc, E, Lookup);
if (Awaitable.isInvalid())
----------------
(aside, this variable name is really unfortunate: I think `E` here is the
awaitable, and `Awaitable` is the _awaiter_. If this is right, feel free to
change it or not...)
================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:865
-ExprResult Sema::BuildResolvedCoawaitExpr(SourceLocation Loc, Expr *E,
- bool IsImplicit) {
+ExprResult Sema::BuildResolvedCoawaitExpr(SourceLocation Loc, Expr *Operand,
+ Expr *E, bool IsImplicit) {
----------------
Not really your fault, but having two Exprs `Operand` and `E` is terribly
confusing in isolation. I think `E` is `Awaiter` in standardese. Again feel
free to change or not.
================
Comment at: clang/lib/Sema/TreeTransform.h:7934
TreeTransform<Derived>::TransformCoawaitExpr(CoawaitExpr *E) {
- ExprResult Result = getDerived().TransformInitializer(E->getOperand(),
- /*NotCopyInit*/false);
- if (Result.isInvalid())
+ // XXX is transforming the operand and the common-expr separately the
+ // right thing to do?
----------------
Ah, this doesn't *sound* right - it's going to create duplicate subexprs I
think, and we should really try to have the operand still be a subexpr of the
commonexpr like in the non-instantiated case.
TransformInitListExpr() is a fairly similar case, and it just discards the
semantic form and rebuilds from the syntactic one. TransformPseudoObjectExpr
seems to be similar.
I suppose the analogous thing to do here would be to have RebuildCoawaitExpr
call Build**Un**resolvedCoawaitExpr with the operand only, but this is a large
change! (And suggests maybe we should stop building the semantic forms of
dependent coawaits entirely, though that probably would regress diagnostics).
Maybe worth giving this a spin anyway?
@rsmith, care to weight in here?
================
Comment at: clang/lib/Sema/TreeTransform.h:7947
// Always rebuild; we don't know if this needs to be injected into a new
// context or if the promise type has changed.
----------------
(FWIW I don't know what "injected into a new context" means, but I don't think
the promise type can change since DependentCoawaitExpr was added in
20f25cb6dfb3364847f4c570b1914fe51e585def.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115187/new/
https://reviews.llvm.org/D115187
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
