[PATCH] D80109: [AST][RecoveryExpr] Preserve type for broken member call expr.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:14083
     OverloadCandidateSet::iterator Best;
+    bool Succeeded = true;
     switch (CandidateSet.BestViableFunction(*this, UnresExpr->getBeginLoc(),
----------------
seems like `bool Success = false` would be a "safer" default and have to be set in fewer places


================
Comment at: clang/lib/Sema/SemaOverload.cpp:14105
 
     case OR_No_Viable_Function:
       CandidateSet.NoteCandidates(
----------------
hokein wrote:
> It is a bit sad that the broken function call cases (too many/few agguments) are failing into this group, all candidates are not viable -- this means we don't get any benefits (no `Best`), for some cases, it is suboptimal even though the best candidate looks like obvious.
> 
> ```
> class Collection {
>   const char *find(int) const;
>   char* find(int);
> };
> void test1(const Collection &C) {
>  C.find(); // we want const char*, but all overloads are not viable and return types are not the same, so no type captured here.
> }
> ```
Yeah, at some point we can add more heuristics (discard non-viable or hidden functions using heuristics) to handle this case, like I did in SemaCodeComplete...


================
Comment at: clang/lib/Sema/SemaOverload.cpp:14133
     }
+    // Overload resolvation fails, try to recover.
+    if (!Succeeded)
----------------
resolvation -> resolution


================
Comment at: clang/test/AST/ast-dump-recovery.cpp:123
+
+  // FIXME: capture the type!
+  f.func(1);
----------------
why does this not work?
isn't there only one candidate, so chooseRecoveryType should pick it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80109