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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
