[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

[David Blaikie via Phabricator via cfe-commits]

dblaikie marked an inline comment as done.
dblaikie added a comment.

In D61357#1488839 <https://reviews.llvm.org/D61357#1488839>, @rsmith wrote:

> In D61357#1485581 <https://reviews.llvm.org/D61357#1485581>, @dblaikie wrote:
>
> > Oh, @rsmith - if there's any better/different testing (if you can figure out how to reduce the test case down further, now that we know the cause - or if you'd like testing for other codepaths I've touched in this patch) I'm all ears. (also naming/API wrangling - my changes were just the minimal sort of "this works" based on what we discussed, but totally happy to do more involved/different things if there's such to be done)
>
>
> Perhaps we could add a diagnostic scope object of some kind, which would assert (or reorder the diagnostics?) if a diagnostic is produced while producing notes for a different diagnostic. That might help for new code and for places where we're aware -- or suspect -- there is a problem, but we don't know where the existing bugs are and that doesn't help us find them. :(


Yeah, I was thinking about something like that. Yeah, doesn't seem any reason we couldn't retrofit something on top of the diagnostics infratsructure - or a utility in it that connects notes to their diagnostic & either asserts, or delays emission, etc.

I was thinking about that in the context of cleaning up /everything/ to use that, but yeah - we could do it as an incremental migration/as needed sort of thing.



================
Comment at: lib/Sema/SemaOverload.cpp:3518-3519
           << false << From->getType() << From->getSourceRange() << ToType;
   } else
     return false;
+
----------------
rsmith wrote:
> Can we avoid calling `CompleteCandidates` on this path?
Yeah, I raised up the two if conditions to a pre-check:

    if (!(OvResult == OR_Ambiguous ||
          (OvResult == OR_No_Viable_Function && !CandidateSet.empty())))
      return false;

    auto Cands = ...
    if (...Ambiguous)
      ...
    else { // No_Viable_Function && !empty()
      ...
    NoteCandidates

(open to other ideas, couldn't quite think of anything that didn't involve some duplication - either duplicating the conditions or the CompleteCandidates call, etc)


Repository:
  rC Clang

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

https://reviews.llvm.org/D61357