sammccall added a comment. The thrust of this change is great, I'm just having trouble being sure there are no bad side-effects.
--- I almost left a comment here saying we might want to recover here... before noticing that we recover all the way up at ParsePostfixExpressionSuffix(). It may be worth a comment on the function implementation that RecoveryExpr fallback is done at a higher level. But on this note, I think we have to consider the effect on *other* callsites. For example `Sema::tryExprAsCall` checks if the result is non-null in order to produce "function must be callled, did you mean to call with no arguments"... but now I think we're returning non-null even if args are required. The fix there is to check containsErrors too. There are a bunch of "internal" uses to Sema::BuildCallExpr (which calls this function), like coroutines, decomposition, pseudo-object etc that might need to be checked. Finally BuildRecoveryCallExpr in SemaOverload itself calls BuildCallExpr, and maybe we can end up with a RecoveryExpr wrapping a RecoveryExpr. --- I'm not sure what the most principled thing to do here is. Recovering (only) at a higher level seems sensible, but then we lack a way to propagate the type. Checking for containsErrors() where needed seems fragile. Adding a new state to ExprResult (null/error/Expr/QualType) is a pretty big change that may not be useful in general. I'm not opposed to this patch if we carefully audit all callers but this doesn't feel great. Maybe an ugly "allowRecovery" flag to BuildCallExpr is the right compromise, at least it forces us to be explicit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92298/new/ https://reviews.llvm.org/D92298 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
