[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Justin Lebar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc90dac27e94e: [clang] Print 32 candidates on the first 
failure, with -fshow-overloads=best. (authored by jlebar).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95754

Files:
  clang/include/clang/Basic/Diagnostic.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp
  clang/test/SemaCXX/overloaded-builtin-operators.cpp

Index: clang/test/SemaCXX/overloaded-builtin-operators.cpp
===
--- clang/test/SemaCXX/overloaded-builtin-operators.cpp
+++ clang/test/SemaCXX/overloaded-builtin-operators.cpp
@@ -195,8 +195,7 @@
 
 void test_dr425(A a) {
   (void)(1.0f * a); // expected-error{{ambiguous}} \
-// expected-note 4{{candidate}} \
-// expected-note {{remaining 8 candidates omitted; pass -fshow-overloads=all to show them}}
+// expected-note 12{{candidate}}
 }
 
 // pr5432
Index: clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp
===
--- clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp
+++ clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp
@@ -10,9 +10,20 @@
   S(signed int*);
 };
 void f(const S& s);
-void g() {
-  f(0);
-}
+
+// First call to f emits all candidates.  Second call emits just the first 4.
+void g() { f(0); }
+// CHECK: {{conversion from 'int' to 'const S' is ambiguous}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+
+void h() { f(0); }
 // CHECK: {{conversion from 'int' to 'const S' is ambiguous}}
 // CHECK-NEXT: {{candidate constructor}}
 // CHECK-NEXT: {{candidate constructor}}
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -10355,18 +10355,15 @@
  const PartialDiagnostic ) const {
   S.Diag(CaretLoc, PDiag)
 << Ambiguous.getFromType() << Ambiguous.getToType();
-  // FIXME: The note limiting machinery is borrowed from
-  // OverloadCandidateSet::NoteCandidates; there's an opportunity for
-  // refactoring here.
-  const OverloadsShown ShowOverloads = S.Diags.getShowOverloads();
   unsigned CandsShown = 0;
   AmbiguousConversionSequence::const_iterator I, E;
   for (I = Ambiguous.begin(), E = Ambiguous.end(); I != E; ++I) {
-if (CandsShown >= 4 && ShowOverloads == Ovl_Best)
+if (CandsShown >= S.Diags.getNumOverloadCandidatesToShow())
   break;
 ++CandsShown;
 S.NoteOverloadCandidate(I->first, I->second);
   }
+  S.Diags.overloadCandidatesShown(CandsShown);
   if (I != E)
 S.Diag(SourceLocation(), diag::note_ovl_too_many_candidates) << int(E - I);
 }
@@ -11644,7 +11641,7 @@
  (Cand.Function->template hasAttr() &&
   Cand.Function->template hasAttr());
 });
-DeferHint = WrongSidedCands.size();
+DeferHint = !WrongSidedCands.empty();
   }
   return DeferHint;
 }
@@ -11677,10 +11674,8 @@
   for (; I != E; ++I) {
 OverloadCandidate *Cand = *I;
 
-// Set an arbitrary limit on the number of candidate functions we'll spam
-// the user with.  FIXME: This limit should depend on details of the
-// candidate list.
-if (CandsShown >= 4 && ShowOverloads == Ovl_Best) {
+if (CandsShown >= S.Diags.getNumOverloadCandidatesToShow() &&
+ShowOverloads == Ovl_Best) {
   break;
 }
 ++CandsShown;
@@ -11709,6 +11704,10 @@
 }
   }
 
+  // Inform S.Diags that we've shown an overload set with N elements.  This may
+  // inform the future value of S.Diags.getNumOverloadCandidatesToShow().
+  S.Diags.overloadCandidatesShown(CandsShown);
+
   if (I != E)
 S.Diag(OpLoc, diag::note_ovl_too_many_candidates,
shouldDeferDiags(S, Args, OpLoc))
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -2312,9 +2312,7 @@
   int SuppressedOverloads = 0;
   for (UnresolvedSetImpl::iterator It = Overloads.begin(),
DeclsEnd = Overloads.end(); It != DeclsEnd; ++It) {
-// FIXME: Magic number for max shown overloads stolen from
-// OverloadCandidateSet::NoteCandidates.
-if (ShownOverloads >= 4 && S.Diags.getShowOverloads() == Ovl_Best) {
+if (ShownOverloads >= S.Diags.getNumOverloadCandidatesToShow()) {
   ++SuppressedOverloads;
   continue;
 }
@@ -2330,6 +2328,8 @@
 

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Justin Lebar via Phabricator via cfe-commits]

jlebar added a comment.

Thank you for the review!

I'll put a note in my cal to land this in a few days if I don't hear from 
@rsmith


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95754

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert accepted this revision.
aaronpuchert added a comment.
This revision is now accepted and ready to land.

Thanks, this looks good to me. Maybe wait a few days whether @rsmith has a 
comment before you land it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95754

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Justin Lebar via Phabricator via cfe-commits]

jlebar added a comment.

> I guess I was confused by the function name

Haha, two hardest problems in computer science.  :)

Changed it to `S.Diags.overloadCandidatesShown(ShownOverloads);`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95754

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Justin Lebar via Phabricator via cfe-commits]

jlebar updated this revision to Diff 325158.
jlebar added a comment.

Rename noteNumOverloadCandidatesShown -> overloadCandidatesShown.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95754

Files:
  clang/include/clang/Basic/Diagnostic.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp
  clang/test/SemaCXX/overloaded-builtin-operators.cpp

Index: clang/test/SemaCXX/overloaded-builtin-operators.cpp
===
--- clang/test/SemaCXX/overloaded-builtin-operators.cpp
+++ clang/test/SemaCXX/overloaded-builtin-operators.cpp
@@ -195,8 +195,7 @@
 
 void test_dr425(A a) {
   (void)(1.0f * a); // expected-error{{ambiguous}} \
-// expected-note 4{{candidate}} \
-// expected-note {{remaining 8 candidates omitted; pass -fshow-overloads=all to show them}}
+// expected-note 12{{candidate}}
 }
 
 // pr5432
Index: clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp
===
--- clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp
+++ clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp
@@ -10,9 +10,20 @@
   S(signed int*);
 };
 void f(const S& s);
-void g() {
-  f(0);
-}
+
+// First call to f emits all candidates.  Second call emits just the first 4.
+void g() { f(0); }
+// CHECK: {{conversion from 'int' to 'const S' is ambiguous}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+
+void h() { f(0); }
 // CHECK: {{conversion from 'int' to 'const S' is ambiguous}}
 // CHECK-NEXT: {{candidate constructor}}
 // CHECK-NEXT: {{candidate constructor}}
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -10354,18 +10354,15 @@
  const PartialDiagnostic ) const {
   S.Diag(CaretLoc, PDiag)
 << Ambiguous.getFromType() << Ambiguous.getToType();
-  // FIXME: The note limiting machinery is borrowed from
-  // OverloadCandidateSet::NoteCandidates; there's an opportunity for
-  // refactoring here.
-  const OverloadsShown ShowOverloads = S.Diags.getShowOverloads();
   unsigned CandsShown = 0;
   AmbiguousConversionSequence::const_iterator I, E;
   for (I = Ambiguous.begin(), E = Ambiguous.end(); I != E; ++I) {
-if (CandsShown >= 4 && ShowOverloads == Ovl_Best)
+if (CandsShown >= S.Diags.getNumOverloadCandidatesToShow())
   break;
 ++CandsShown;
 S.NoteOverloadCandidate(I->first, I->second);
   }
+  S.Diags.overloadCandidatesShown(CandsShown);
   if (I != E)
 S.Diag(SourceLocation(), diag::note_ovl_too_many_candidates) << int(E - I);
 }
@@ -11643,7 +11640,7 @@
  (Cand.Function->template hasAttr() &&
   Cand.Function->template hasAttr());
 });
-DeferHint = WrongSidedCands.size();
+DeferHint = !WrongSidedCands.empty();
   }
   return DeferHint;
 }
@@ -11676,10 +11673,8 @@
   for (; I != E; ++I) {
 OverloadCandidate *Cand = *I;
 
-// Set an arbitrary limit on the number of candidate functions we'll spam
-// the user with.  FIXME: This limit should depend on details of the
-// candidate list.
-if (CandsShown >= 4 && ShowOverloads == Ovl_Best) {
+if (CandsShown >= S.Diags.getNumOverloadCandidatesToShow() &&
+ShowOverloads == Ovl_Best) {
   break;
 }
 ++CandsShown;
@@ -11708,6 +11703,10 @@
 }
   }
 
+  // Inform S.Diags that we've shown an overload set with N elements.  This may
+  // inform the future value of S.Diags.getNumOverloadCandidatesToShow().
+  S.Diags.overloadCandidatesShown(CandsShown);
+
   if (I != E)
 S.Diag(OpLoc, diag::note_ovl_too_many_candidates,
shouldDeferDiags(S, Args, OpLoc))
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -2291,9 +2291,7 @@
   int SuppressedOverloads = 0;
   for (UnresolvedSetImpl::iterator It = Overloads.begin(),
DeclsEnd = Overloads.end(); It != DeclsEnd; ++It) {
-// FIXME: Magic number for max shown overloads stolen from
-// OverloadCandidateSet::NoteCandidates.
-if (ShownOverloads >= 4 && S.Diags.getShowOverloads() == Ovl_Best) {
+if (ShownOverloads >= S.Diags.getNumOverloadCandidatesToShow()) {
   ++SuppressedOverloads;
   continue;
 }
@@ -2309,6 +2307,8 @@
 ++ShownOverloads;
   }
 
+  

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:2310
 
+  S.Diags.noteNumOverloadCandidatesShown(ShownOverloads);
+

jlebar wrote:
> aaronpuchert wrote:
> > jlebar wrote:
> > > aaronpuchert wrote:
> > > > Why not in the following `if`? I assume we want to show a long list not 
> > > > necessarily once, but only if it comes with the first error?
> > > My intent was to show the long list once, even if it's not the very first 
> > > error.  My thought process:
> > > 
> > > All things being equal, it's better to show more information to the user 
> > > than less.  The problem is, at some point, the amount of information we 
> > > show becomes overwhelming and spammy.  Particularly problematic are 
> > > multiline errors, because then you get O(nm) error lines across the whole 
> > > TU.  We prevent the O(nm) overwhelm by limiting the number of lines a 
> > > particular error can produce (using the mechanism in question here, or 
> > > the template backtrace limit, etc), and then also limiting the total 
> > > number of individual errors before we stop printing those.
> > > 
> > > With this change, we display the full(ish) error the first time it occurs 
> > > and then the truncated error every other time.  So in total it's O(n + m) 
> > > rather than O(nm).
> > Got it, but currently you'd not be exhausting the option to print a long 
> > list once: when the first overload resolution error has fewer candidates 
> > than the limit you'd still say we stop printing long lists of notes from 
> > now on. That confused me because you're calling 
> > `noteNumOverloadCandidatesShown` but you might not actually have printed 
> > that note.
> > 
> > If you're going by the //O(n+m)// argument, you could put this call into 
> > the `if (SuppressedOverloads)` and still stay under that limit.
> > currently you'd not be exhausting the option to print a long list once: 
> > when the first overload resolution error has fewer candidates than the 
> > limit you'd still say we stop printing long lists of notes from now on.
> 
> But if we print <= 4 overloads then noteNumOverloadCandidatesShown has no 
> effect?
> 
> OTOH if we move it into the `if` then imagine a case where NumOverloads 
> starts at 32 and we print 16 overloads.  In that case we don't suppress any 
> overloads.  But the *next* time we still want to limit it to only 4.  So 
> moving it into the `if` would do the wrong thing.
> 
> Unless I'm misunderstanding?
You're absolutely right. I guess I was confused by the function name 
`noteNumOverloadCandidatesShown`, thinking that calling it would indicate that 
we've shown the note below.

Perhaps we can just drop the `note` or find another verb? Or you could explain 
how the name is intended to be read.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95754

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Justin Lebar via Phabricator via cfe-commits]

jlebar added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:2310
 
+  S.Diags.noteNumOverloadCandidatesShown(ShownOverloads);
+

aaronpuchert wrote:
> jlebar wrote:
> > aaronpuchert wrote:
> > > Why not in the following `if`? I assume we want to show a long list not 
> > > necessarily once, but only if it comes with the first error?
> > My intent was to show the long list once, even if it's not the very first 
> > error.  My thought process:
> > 
> > All things being equal, it's better to show more information to the user 
> > than less.  The problem is, at some point, the amount of information we 
> > show becomes overwhelming and spammy.  Particularly problematic are 
> > multiline errors, because then you get O(nm) error lines across the whole 
> > TU.  We prevent the O(nm) overwhelm by limiting the number of lines a 
> > particular error can produce (using the mechanism in question here, or the 
> > template backtrace limit, etc), and then also limiting the total number of 
> > individual errors before we stop printing those.
> > 
> > With this change, we display the full(ish) error the first time it occurs 
> > and then the truncated error every other time.  So in total it's O(n + m) 
> > rather than O(nm).
> Got it, but currently you'd not be exhausting the option to print a long list 
> once: when the first overload resolution error has fewer candidates than the 
> limit you'd still say we stop printing long lists of notes from now on. That 
> confused me because you're calling `noteNumOverloadCandidatesShown` but you 
> might not actually have printed that note.
> 
> If you're going by the //O(n+m)// argument, you could put this call into the 
> `if (SuppressedOverloads)` and still stay under that limit.
> currently you'd not be exhausting the option to print a long list once: when 
> the first overload resolution error has fewer candidates than the limit you'd 
> still say we stop printing long lists of notes from now on.

But if we print <= 4 overloads then noteNumOverloadCandidatesShown has no 
effect?

OTOH if we move it into the `if` then imagine a case where NumOverloads starts 
at 32 and we print 16 overloads.  In that case we don't suppress any overloads. 
 But the *next* time we still want to limit it to only 4.  So moving it into 
the `if` would do the wrong thing.

Unless I'm misunderstanding?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95754

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:2310
 
+  S.Diags.noteNumOverloadCandidatesShown(ShownOverloads);
+

jlebar wrote:
> aaronpuchert wrote:
> > Why not in the following `if`? I assume we want to show a long list not 
> > necessarily once, but only if it comes with the first error?
> My intent was to show the long list once, even if it's not the very first 
> error.  My thought process:
> 
> All things being equal, it's better to show more information to the user than 
> less.  The problem is, at some point, the amount of information we show 
> becomes overwhelming and spammy.  Particularly problematic are multiline 
> errors, because then you get O(nm) error lines across the whole TU.  We 
> prevent the O(nm) overwhelm by limiting the number of lines a particular 
> error can produce (using the mechanism in question here, or the template 
> backtrace limit, etc), and then also limiting the total number of individual 
> errors before we stop printing those.
> 
> With this change, we display the full(ish) error the first time it occurs and 
> then the truncated error every other time.  So in total it's O(n + m) rather 
> than O(nm).
Got it, but currently you'd not be exhausting the option to print a long list 
once: when the first overload resolution error has fewer candidates than the 
limit you'd still say we stop printing long lists of notes from now on. That 
confused me because you're calling `noteNumOverloadCandidatesShown` but you 
might not actually have printed that note.

If you're going by the //O(n+m)// argument, you could put this call into the 
`if (SuppressedOverloads)` and still stay under that limit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95754

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Justin Lebar via Phabricator via cfe-commits]

jlebar added a comment.

Thank you for your comments, Aaron!




Comment at: clang/lib/Sema/Sema.cpp:2310
 
+  S.Diags.noteNumOverloadCandidatesShown(ShownOverloads);
+

aaronpuchert wrote:
> Why not in the following `if`? I assume we want to show a long list not 
> necessarily once, but only if it comes with the first error?
My intent was to show the long list once, even if it's not the very first 
error.  My thought process:

All things being equal, it's better to show more information to the user than 
less.  The problem is, at some point, the amount of information we show becomes 
overwhelming and spammy.  Particularly problematic are multiline errors, 
because then you get O(nm) error lines across the whole TU.  We prevent the 
O(nm) overwhelm by limiting the number of lines a particular error can produce 
(using the mechanism in question here, or the template backtrace limit, etc), 
and then also limiting the total number of individual errors before we stop 
printing those.

With this change, we display the full(ish) error the first time it occurs and 
then the truncated error every other time.  So in total it's O(n + m) rather 
than O(nm).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95754

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D95754#2561849 , @jlebar wrote:

> Not sure who can review this, but looking through blame it seems like maybe 
> @aaronpuchert?

I'm by no means an expert on overloading, but this seems more like a usability 
question.

I like the idea. Let's give @rsmith a chance to chime in, otherwise I'll 
approve it after a while.




Comment at: clang/lib/Sema/Sema.cpp:2310
 
+  S.Diags.noteNumOverloadCandidatesShown(ShownOverloads);
+

Why not in the following `if`? I assume we want to show a long list not 
necessarily once, but only if it comes with the first error?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95754

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Justin Lebar via Phabricator via cfe-commits]

jlebar added a comment.

Not sure who can review this, but looking through blame it seems like maybe 
@aaronpuchert?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95754

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Justin Lebar via Phabricator via cfe-commits]

jlebar created this revision.
jlebar requested review of this revision.
Herald added a project: clang.

Previously, -fshow-overloads=best always showed 4 candidates.  The
problem is, when this isn't enough, you're kind of up a creek; the only
option available is to recompile with different flags.  This can be
quite expensive!

With this change, we try to strike a compromise.  The *first* error with
more than 4 candidates will show up to 32 candidates.  All further
errors continue to show only 4 candidates.

The hope is that this way, users will have *some chance* of making
forward progress, without facing unbounded amounts of error spam.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95754

Files:
  clang/include/clang/Basic/Diagnostic.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp
  clang/test/SemaCXX/overloaded-builtin-operators.cpp

Index: clang/test/SemaCXX/overloaded-builtin-operators.cpp
===
--- clang/test/SemaCXX/overloaded-builtin-operators.cpp
+++ clang/test/SemaCXX/overloaded-builtin-operators.cpp
@@ -195,8 +195,7 @@
 
 void test_dr425(A a) {
   (void)(1.0f * a); // expected-error{{ambiguous}} \
-// expected-note 4{{candidate}} \
-// expected-note {{remaining 8 candidates omitted; pass -fshow-overloads=all to show them}}
+// expected-note 12{{candidate}}
 }
 
 // pr5432
Index: clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp
===
--- clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp
+++ clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp
@@ -10,9 +10,20 @@
   S(signed int*);
 };
 void f(const S& s);
-void g() {
-  f(0);
-}
+
+// First call to f emits all candidates.  Second call emits just the first 4.
+void g() { f(0); }
+// CHECK: {{conversion from 'int' to 'const S' is ambiguous}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+
+void h() { f(0); }
 // CHECK: {{conversion from 'int' to 'const S' is ambiguous}}
 // CHECK-NEXT: {{candidate constructor}}
 // CHECK-NEXT: {{candidate constructor}}
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -10354,18 +10354,15 @@
  const PartialDiagnostic ) const {
   S.Diag(CaretLoc, PDiag)
 << Ambiguous.getFromType() << Ambiguous.getToType();
-  // FIXME: The note limiting machinery is borrowed from
-  // OverloadCandidateSet::NoteCandidates; there's an opportunity for
-  // refactoring here.
-  const OverloadsShown ShowOverloads = S.Diags.getShowOverloads();
   unsigned CandsShown = 0;
   AmbiguousConversionSequence::const_iterator I, E;
   for (I = Ambiguous.begin(), E = Ambiguous.end(); I != E; ++I) {
-if (CandsShown >= 4 && ShowOverloads == Ovl_Best)
+if (CandsShown >= S.Diags.getNumOverloadCandidatesToShow())
   break;
 ++CandsShown;
 S.NoteOverloadCandidate(I->first, I->second);
   }
+  S.Diags.noteNumOverloadCandidatesShown(CandsShown);
   if (I != E)
 S.Diag(SourceLocation(), diag::note_ovl_too_many_candidates) << int(E - I);
 }
@@ -11643,7 +11640,7 @@
  (Cand.Function->template hasAttr() &&
   Cand.Function->template hasAttr());
 });
-DeferHint = WrongSidedCands.size();
+DeferHint = !WrongSidedCands.empty();
   }
   return DeferHint;
 }
@@ -11676,10 +11673,8 @@
   for (; I != E; ++I) {
 OverloadCandidate *Cand = *I;
 
-// Set an arbitrary limit on the number of candidate functions we'll spam
-// the user with.  FIXME: This limit should depend on details of the
-// candidate list.
-if (CandsShown >= 4 && ShowOverloads == Ovl_Best) {
+if (CandsShown >= S.Diags.getNumOverloadCandidatesToShow() &&
+ShowOverloads == Ovl_Best) {
   break;
 }
 ++CandsShown;
@@ -11708,6 +11703,10 @@
 }
   }
 
+  // Inform S.Diags that we've shown an overload set with N elements.  This may
+  // inform the future value of S.Diags.getNumOverloadCandidatesToShow().
+  S.Diags.noteNumOverloadCandidatesShown(CandsShown);
+
   if (I != E)
 S.Diag(OpLoc, diag::note_ovl_too_many_candidates,
shouldDeferDiags(S, Args, OpLoc))
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -2291,9 +2291,7 @@
   int SuppressedOverloads = 0;
   for (UnresolvedSetImpl::iterator It = Overloads.begin(),