[PATCH] D89946: [clang] Suppress "follow-up" diagnostics on recovery call expressions.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:12814
   // end up here.
   return SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
                                MultiExprArg(Args.data(), Args.size()),
----------------
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > This results in (IIUC):
> > > 
> > > ```
> > > CallExpr
> > > >RecoveryExpr (indirection we inserted)
> > > >>DeclRefExpr (callee)
> > > >Arg1
> > > ```
> > > 
> > > Whereas when overload resolution fails, we create:
> > > ```
> > > RecoveryExpr (call)
> > > > OverloadExpr (callee)
> > > > Arg1
> > > ```
> > > 
> > > I can see advantages for either, but is there a reason not to be consistent?
> > > (Which I guess means emitting the first one here)
> > > (Which I guess means emitting the first one here)
> > 
> > yes, exactly. 
> > 
> > reasons:
> > - BuildCallExpr could emit diagnostics if we pass a non-dependent callee to it, this is something we want to avoid;
> > - we'd save some cost -- `BuildCallExpr` has dedicated code path to handle dependent expr; 
> Sorry, I meant emitting the second one here. (Or changing the callexpr recovery to use the first form, but that's a larger patch).
> 
> I understand we save some code, but not very much (the second form is easy to produce), and we produce quite different ASTs for essentially the same pattern (the only difference is whether we attempted recovery or not). This doesn't seem like a good trade...
Changed to the second form for consistency. And it doesn't break any existing tests (neither improvements nor regressions).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89946