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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Looks good to me - this still seems hairy and abstract to me so I'd "expect the unexpected" in terms of bot or downstream breakage...



================
Comment at: clang/include/clang/Sema/Sema.h:10394
+                                      UnresolvedLookupExpr *Lookup);
+  ExprResult BuildResolvedCoawaitExpr(SourceLocation KwLoc, Expr *Operand,
+                                      Expr *E, bool IsImplicit = false);
----------------
nit: update param names here


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:824
+
+ExprResult Sema::BuildUnresolvedCoawaitExpr(SourceLocation Loc, Expr *Operand,
                                             UnresolvedLookupExpr *Lookup) {
----------------
this name is a bit confusing - the result isn't (necessarily) unresolved, it's more like the inputs are resolved.

I don't actually think we should rename it, but maybe add a comment like `Attempts to resolve and build a CoAwaitExpr from "raw" inputs, bailing out to DependentCoawaitExpr if needed`

(Not caused by this patch, but I feel like the only time to add comments to make this less confusing is when we've been digging into it)


================
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
----------------
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.


================
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'
----------------
nit: I think it'd be nice to include the type here for consistency


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115187/new/

https://reviews.llvm.org/D115187