[PATCH] D115187: [clangd] Expose CoawaitExpr's operand in the AST

[Nathan Ridge via Phabricator via cfe-commits]

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