sammccall added a comment. In D115187#3500098 <https://reviews.llvm.org/D115187#3500098>, @nridge wrote:
> I think the issue is related to this loop > <https://searchfox.org/llvm/rev/d5d498f9baae218c56dc3a3582ef0083f795f088/clang/lib/Sema/SemaChecking.cpp#14088> > in `AnalyzeImplicitConversions()`, which iterates over `Expr::children()`, > and adds each child to a list of expressions to be checked for implicit > conversions. > > `CoroutineSuspendExpr` now has the operand as an extra child, and an implicit > conversion in the operand gets diagnosed both when processing the operand, > and the common-expr. > > I'm guessing this will need an explicit carve-out in > `AnalyzeImplicitConversions`. There is already some special handling of other > expressions types there, including at least one > <https://searchfox.org/llvm/rev/d5d498f9baae218c56dc3a3582ef0083f795f088/clang/lib/Sema/SemaChecking.cpp#13997> > whose purpose is to avoid duplicate diagnostics. Yeah, I agree. The general issue here is that storing alternate representations that share subexpressions can cause duplicated traversal. I guess having RecursiveASTVisitor do the right thing will cover most cases. PseudoObjectExpr has this pattern (for objc property access), InitListExpr kind of does this (syntactic vs semantic forms). For both, I don't fully understand the mechanism, and it seems to require hacks in lots of places :-( FWIW PseudoObjectExpr avoids duplicated diagnostics here by wrapping all its semantic/derived subexpressions inside `OpaqueValueExpr`, the special case you found... I don't know what the implications of trying to reuse that mechanism would be. The explicit carve-out seems simpler. ================ 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 ---------------- nridge wrote: > 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. Thank you, that makes a lot of sense. Could you add to the comment something like: // This mirrors how the implicit CoAwaitExpr is originally created in Sema::ActOnCoroutineBodyStart? ================ 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' ---------------- nridge wrote: > 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. I'm not sure what I meant either, sorry! Looks good. 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
