Re: [PATCH v2] c++: tweaks for explicit conversion fns diagnostic

[Jason Merrill <jason@redhat.com>]

On 8/28/23 19:24, Marek Polacek wrote:
> On Fri, Aug 25, 2023 at 08:34:37PM -0400, Jason Merrill wrote:
>> On 8/25/23 19:37, Marek Polacek wrote:
>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>>
>>> -- >8 --
>>>
>>> 1) When saying that a conversion is erroneous because it would use
>>> an explicit constructor, it might be nice to show where exactly
>>> the explicit constructor is located.  For example, with this patch:
>>>
>>> [...]
>>> explicit.C:4:12: note: 'S::S(int)' declared here
>>>       4 |   explicit S(int) { }
>>>         |            ^
>>>
>>> 2) When a conversion doesn't work out merely because the conversion
>>> function necessary to do the conversion couldn't be used because
>>> it was marked explicit, it would be useful to the user to say so,
>>> rather than just saying "cannot convert".  For example, with this patch:
>>>
>>> explicit.C:13:12: error: cannot convert 'S' to 'bool' in initialization
>>>      13 |   bool b = S{1};
>>>         |            ^~~~
>>>         |            |
>>>         |            S
>>> explicit.C:5:12: note: explicit conversion function was not considered
>>>       5 |   explicit operator bool() const { return true; }
>>>         |            ^~~~~~~~
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* call.cc (convert_like_internal): Show where the conversion function
>>> 	was declared.
>>> 	(maybe_show_nonconverting_candidate): New.
>>> 	* cp-tree.h (maybe_show_nonconverting_candidate): Declare.
>>> 	* typeck.cc (convert_for_assignment): Call it.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/diagnostic/explicit.C: New test.
>>> ---
>>>    gcc/cp/call.cc                             | 41 +++++++++++++++++++---
>>>    gcc/cp/cp-tree.h                           |  1 +
>>>    gcc/cp/typeck.cc                           |  5 +++
>>>    gcc/testsuite/g++.dg/diagnostic/explicit.C | 16 +++++++++
>>>    4 files changed, 59 insertions(+), 4 deletions(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/diagnostic/explicit.C
>>>
>>> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
>>> index 23e458d3252..09ebcf6a115 100644
>>> --- a/gcc/cp/call.cc
>>> +++ b/gcc/cp/call.cc
>>> @@ -8459,12 +8459,21 @@ convert_like_internal (conversion *convs, tree expr, tree fn, int argnum,
>>>    		if (pedwarn (loc, 0, "converting to %qT from initializer list "
>>>    			     "would use explicit constructor %qD",
>>>    			     totype, convfn))
>>> -		  inform (loc, "in C++11 and above a default constructor "
>>> -			  "can be explicit");
>>> +		  {
>>> +		    inform (loc, "in C++11 and above a default constructor "
>>> +			    "can be explicit");
>>> +		    inform (DECL_SOURCE_LOCATION (convfn), "%qD declared here",
>>> +			    convfn);
>>
>> I'd swap these two informs.
> 
> Done.
>   
>>> +++ b/gcc/testsuite/g++.dg/diagnostic/explicit.C
>>> @@ -0,0 +1,16 @@
>>> +// { dg-do compile { target c++11 } }
>>> +
>>> +struct S {
>>> +  explicit S(int) { }
>>> +  explicit operator bool() const { return true; } // { dg-message "explicit conversion function was not considered" }
>>> +  explicit operator int() const { return 42; } // { dg-message "explicit conversion function was not considered" }
>>> +};
>>> +
>>> +void
>>> +g ()
>>> +{
>>> +  S s = {1}; // { dg-error "would use explicit constructor" }
>>> +  bool b = S{1}; // { dg-error "cannot convert .S. to .bool. in initialization" }
>>> +  int i;
>>> +  i = S{2}; // { dg-error "cannot convert .S. to .int. in assignment" }
>>> +}
>>
>> Let's also test other copy-initialization contexts: parameter passing,
>> return, throw, aggregate member initialization.
> 
> Done except for throw.  To handle arg passing I moved the call to
> maybe_show_nonconverting_candidate one line down.  I guess a testcase
> for throw would be
> 
> struct T {
>    T() { } // #1
>    explicit T(const T&) { } // #2
> };
> 
> void
> g ()
> {
>    T t{};
>    throw t;
> }
> 
> but #2 isn't a viable candidate so this would take more effort to handle.

True, copy-initialization is different when the types are the same.

> We just say about #1 that "candidate expects 0 arguments, 1 provided".
> 
> clang++ says
> 
> e.C:3:12: note: explicit constructor is not a candidate
>      3 |   explicit T(const T&) { }
>        |            ^

That would be better; in add_candidates when we see an explicit 
constructor we could add it to bad_fns instead of ignoring it.  But that 
doesn't need to be part of this patch.

> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> -- >8 --
> 1) When saying that a conversion is erroneous because it would use
> an explicit constructor, it might be nice to show where exactly
> the explicit constructor is located.  For example, with this patch:
> 
> [...]
> explicit.C:4:12: note: 'S::S(int)' declared here
>      4 |   explicit S(int) { }
>        |            ^
> 
> 2) When a conversion doesn't work out merely because the conversion
> function necessary to do the conversion couldn't be used because
> it was marked explicit, it would be useful to the user to say so,
> rather than just saying "cannot convert".  For example, with this patch:
> 
> explicit.C:13:12: error: cannot convert 'S' to 'bool' in initialization
>     13 |   bool b = S{1};
>        |            ^~~~
>        |            |
>        |            S
> explicit.C:5:12: note: explicit conversion function was not considered
>      5 |   explicit operator bool() const { return true; }
>        |            ^~~~~~~~
> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (convert_like_internal): Show where the conversion function
> 	was declared.
> 	(maybe_show_nonconverting_candidate): New.
> 	* cp-tree.h (maybe_show_nonconverting_candidate): Declare.
> 	* typeck.cc (convert_for_assignment): Call it.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/diagnostic/explicit.C: New test.
> ---
>   gcc/cp/call.cc                             | 41 +++++++++++++++++++---
>   gcc/cp/cp-tree.h                           |  1 +
>   gcc/cp/typeck.cc                           |  6 ++++
>   gcc/testsuite/g++.dg/diagnostic/explicit.C | 33 +++++++++++++++++
>   4 files changed, 77 insertions(+), 4 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/diagnostic/explicit.C
> 
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index 23e458d3252..52c9f4265a4 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -14323,4 +14332,28 @@ is_list_ctor (tree decl)
>     return true;
>   }
>   
> +/* We know that can_convert_arg_bad already said "no" when trying to convert
> +   FROM to TO with ARG and FLAGS.  Try to figure out if it was because
> +   an explicit conversion function was skipped when looking for a way to
> +   perform the conversion.  At this point we've already printed an error.  */
> +
> +void
> +maybe_show_nonconverting_candidate (tree to, tree from, tree arg, int flags)
> +{
> +  if (!(flags & LOOKUP_ONLYCONVERTING))
> +    return;
> +
> +  conversion_obstack_sentinel cos;
> +  conversion *c = implicit_conversion (to, from, arg, /*c_cast_p=*/false,
> +				       flags & ~LOOKUP_ONLYCONVERTING, tf_none);
> +  if (c && !c->bad_p && c->user_conv_p)
> +    /* Ay, the conversion would have worked in copy-init context.  */

s/copy/direct/

OK with that change.

Jason