[cfe-commits] r167596 - in /cfe/trunk: lib/Sema/SemaOverload.cpp test/SemaCXX/ambiguous-conversion-show-overload.cpp

[Matt Beaumont-Gay]

Thanks for reviewing!

On Sat, Nov 10, 2012 at 7:55 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
> Hi Matt,
>
> On Thu, Nov 8, 2012 at 10:50 PM, Matt Beaumont-Gay <matthewbg at google.com> wrote:
>> Author: matthewbg
>> Date: Thu Nov  8 14:50:02 2012
>> New Revision: 167596
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=167596&view=rev
>> Log:
>> Fix a bug I found while preparing my devmtg talk: When passing NULL to a
>> function that takes a const Foo&, where Foo is convertible from a large number
>> of pointer types, we print ALL the overloads, no matter the setting of
>> -fshow-overloads.
>>
>> There is potential follow-on work in unifying the "print candidates, but not
>> too many" logic between OverloadCandidateSet::NoteCandidates and
>> ImplicitConversionSequence::DiagnoseAmbiguousConversion.
>>
>> Added:
>>     cfe/trunk/test/SemaCXX/ambiguous-conversion-show-overload.cpp
>> Modified:
>>     cfe/trunk/lib/Sema/SemaOverload.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaOverload.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=167596&r1=167595&r2=167596&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaOverload.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaOverload.cpp Thu Nov  8 14:50:02 2012
>> @@ -7992,10 +7992,20 @@
>>                                   const PartialDiagnostic &PDiag) const {
>>    S.Diag(CaretLoc, PDiag)
>>      << Ambiguous.getFromType() << Ambiguous.getToType();
>> -  for (AmbiguousConversionSequence::const_iterator
>> -         I = Ambiguous.begin(), E = Ambiguous.end(); I != E; ++I) {
>> +  // 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)
>> +      break;
>> +    ++CandsShown;
>>      S.NoteOverloadCandidate(*I);
>>    }
>> +  if (I != E)
>> +    S.Diag(SourceLocation(), diag::note_ovl_too_many_candidates) << int(E - I);
>>  }
>
> SourceLocation() is not the best place to attach this since we have CaretLoc.

I think this is debatable. The note here isn't really associated with
any particular location in the user's code; it's essentially giving
flag usage information. Pointing at a particular source location
doesn't add any value, particularly since we already printed a snippet
and caret at that location. Additionally, many other codepaths where
we emit this note via OverloadCandidateSet::NoteOverloads use the
default value (SourceLocation()) for the last parameter to
NoteOverloads.

>From a more practical standpoint, with the following patch, we end up
emitting an extra macro expansion note (which is probably a bug in the
diagnostic printer that we should fix):

diff --git lib/Sema/SemaOverload.cpp lib/Sema/SemaOverload.cpp
index e5a3dee..0a085a9 100644
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -8005,7 +8005,7 @@ void
ImplicitConversionSequence::DiagnoseAmbiguousConversion(
     S.NoteOverloadCandidate(*I);
   }
   if (I != E)
-    S.Diag(SourceLocation(), diag::note_ovl_too_many_candidates) << int(E - I);
+    S.Diag(CaretLoc, diag::note_ovl_too_many_candidates) << int(E - I);
 }

 namespace {
diff --git test/SemaCXX/ambiguous-conversion-show-overload.cpp
test/SemaCXX/ambiguous-conversion-show-overload.cpp
index 6429651..43f5146 100644
--- test/SemaCXX/ambiguous-conversion-show-overload.cpp
+++ test/SemaCXX/ambiguous-conversion-show-overload.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -fshow-overloads=best
-fno-caret-diagnostics %s 2>&1 | FileCheck %s
+#define NULL __null
 struct S {
   S(void*);
   S(char*);
@@ -11,7 +12,7 @@ struct S {
 };
 void f(const S& s);
 void g() {
-  f(0);
+  f(NULL);
 }
 // CHECK: {{conversion from 'int' to 'const S' is ambiguous}}
 // CHECK-NEXT: {{candidate constructor}}

$ clang -fsyntax-only -fshow-overloads=best
ambiguous-conversion-show-overload.cpp
<snip>
ambiguous-conversion-show-overload.cpp:7:3: note: candidate constructor
  S(signed char*);
  ^
ambiguous-conversion-show-overload.cpp:15:5: note: remaining 4
candidates omitted; pass -fshow-overloads=all to show them
  f(NULL);
    ^
ambiguous-conversion-show-overload.cpp:2:14: note: expanded from macro 'NULL'
#define NULL __null
             ^

>
>>  namespace {
>>
>> Added: cfe/trunk/test/SemaCXX/ambiguous-conversion-show-overload.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/ambiguous-conversion-show-overload.cpp?rev=167596&view=auto
>> ==============================================================================
>> --- cfe/trunk/test/SemaCXX/ambiguous-conversion-show-overload.cpp (added)
>> +++ cfe/trunk/test/SemaCXX/ambiguous-conversion-show-overload.cpp Thu Nov  8 14:50:02 2012
>> @@ -0,0 +1,21 @@
>> +// RUN: %clang_cc1 -fsyntax-only -fshow-overloads=best -fno-caret-diagnostics %s 2>&1 | FileCheck %s
>> +struct S {
>> +  S(void*);
>> +  S(char*);
>> +  S(unsigned char*);
>> +  S(signed char*);
>> +  S(unsigned short*);
>> +  S(signed short*);
>> +  S(unsigned int*);
>> +  S(signed int*);
>> +};
>> +void f(const S& s);
>> +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: {{remaining 4 candidates omitted; pass -fshow-overloads=all to show them}}
>
> Why not -verify?

AFAICT, -verify can't handle diagnostics without source locations. I'd
be happy to learn otherwise :)

-Matt