nridge added inline comments.
================
Comment at: clang/lib/Sema/TreeTransform.h:1476
bool IsImplicit) {
- return getSema().BuildResolvedCoawaitExpr(CoawaitLoc, Result, IsImplicit);
+ // This function rebuilds a coawait-expr given its operator.
+ // For an explicit coawait-expr, the rebuild involves the full set
----------------
sammccall wrote:
> This is essentially inlining BuildUnresolvedCoawaitExpr, with the
> await_transform logic dropped, right? I wonder how that compares to adding a
> `bool Transform` to that function.
>
> Such a param would make everything less direct, but it also seems possible
> that the two copies could unexpectedly diverge either now (e.g. could the
> "placeholder type" logic from BuildUnresolved be relevant here?) or more
> likely in the future.
>
> Up to you, I don't feel strongly here, it just seems a little surprising.
> This is essentially inlining BuildUnresolvedCoawaitExpr, with the
> await_transform logic dropped, right?
Not exactly. Rather, it's attempting to replicate the logic
[here](https://searchfox.org/llvm/rev/d5d498f9baae218c56dc3a3582ef0083f795f088/clang/lib/Sema/SemaCoroutine.cpp#734-738),
which is where implicit CoawaitExpr's are built in the first place.
(The only reason I didn't factor out those few lines into a function of their
own is that here we already have the `UnresolvedLookupExpr` for the `operator
coawait`, whereas there it's calling a helper that builds the lookup-expr
followed by calling `Sema::BuildOperatorCoawaitCall()`.)
So, precisely because `BuildUnresolvedCoawaitExpr` does a few extra things like
the "placeholder type" logic that the code we're trying to replicate doesn't, I
think it's safer not to try and reuse that here.
================
Comment at: clang/test/AST/coroutine-locals-cleanup-exp-namespace.cpp:88
// CHECK-NEXT: CoawaitExpr
-// CHECK-NEXT: MaterializeTemporaryExpr {{.*}}
'Task::Awaiter':'Task::Awaiter'
+// CHECK-NEXT: CXXBindTemporaryExpr {{.*}} 'Task' (CXXTemporary
{{.*}})
+// CHECK: MaterializeTemporaryExpr {{.*}}
'Task::Awaiter':'Task::Awaiter'
----------------
sammccall wrote:
> nit: I think it'd be nice to include the type here for consistency
A full line of example output here is:
```
CXXBindTemporaryExpr 0x1898248 <col:14, col:18> 'Task' (CXXTemporary 0x1898248)
```
not sure what type you have in mind besides `'Task'`, which is already there.
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
