nridge added inline comments.
================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:858
}
ExprResult Awaitable = buildOperatorCoawaitCall(*this, Loc, E, Lookup);
if (Awaitable.isInvalid())
----------------
sammccall wrote:
> (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...)
I believe you're right, changed it
================
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) {
----------------
sammccall wrote:
> 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.
Agreed, changed this as well
================
Comment at: clang/lib/Sema/TreeTransform.h:7959
+
+ // FIXME: getCurScope() should not be used during template instantiation.
+ // We should pick up the set of unqualified lookup results for operator
----------------
Note, I copied this comment from
[here](https://searchfox.org/llvm/rev/34a68037ddb4dff972c5d8c599cf5edf08fadf6b/clang/lib/Sema/SemaStmt.cpp#2580).
================
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?
----------------
sammccall wrote:
> 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?
> I suppose the analogous thing to do here would be to have RebuildCoawaitExpr
> call Build**Un**resolvedCoawaitExpr with the operand only,
I gave this a try.
================
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.
----------------
sammccall wrote:
> (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.)
>
Is the implication of this that there are some conditions under which we can
reuse the original CoawaitExpr rather than rebuilding it?
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
