Re: [PATCH] c++: consider built-in operator candidates first

[Jason Merrill <jason@redhat.com>]

On 9/20/21 12:46, Patrick Palka wrote:
> During operator overload resolution, we currently consider non-member
> candidates before built-in candidates.  This didn't make a difference
> before r12-3346, but after this change add_candidates will avoid
> computing excess argument conversions if we've already seen a strictly
> viable candidate, so it's better to consider built-in candidates first.

Doesn't r12-3346 stop considering conversions after it sees a bad one, 
and later return to the bad candidate if there is no strictly viable 
candidate?  How does this patch change that?

Depending on the order of the candidates seems fragile.

> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?
> 
> gcc/cp/ChangeLog:
> 
> 	* call.c (add_operator_candidates): Consider built-in operator
> 	candidates before considering non-member candidates.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/template/conv17.C: Extend test.
> ---
>   gcc/cp/call.c                          | 13 +++++++------
>   gcc/testsuite/g++.dg/template/conv17.C |  7 +++++++
>   2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index c5601d96ab8..c0da083758f 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -6321,7 +6321,6 @@ add_operator_candidates (z_candidate **candidates,
>   			 vec<tree, va_gc> *arglist,
>   			 int flags, tsubst_flags_t complain)
>   {
> -  z_candidate *start_candidates = *candidates;
>     bool ismodop = code2 != ERROR_MARK;
>     tree fnname = ovl_op_identifier (ismodop, ismodop ? code2 : code);
>   
> @@ -6333,6 +6332,12 @@ add_operator_candidates (z_candidate **candidates,
>     if (rewritten && code != EQ_EXPR && code != SPACESHIP_EXPR)
>       flags &= ~LOOKUP_REWRITTEN;
>   
> +  /* Add built-in candidates to the candidate set.  The standard says to
> +     rewrite built-in candidates, too, but there's no point.  */
> +  if (!rewritten)
> +    add_builtin_candidates (candidates, code, code2, fnname, arglist,
> +			    flags, complain);
> +
>     bool memonly = false;
>     switch (code)
>       {
> @@ -6352,6 +6357,7 @@ add_operator_candidates (z_candidate **candidates,
>   
>     /* Add namespace-scope operators to the list of functions to
>        consider.  */
> +  z_candidate *start_candidates = *candidates;
>     if (!memonly)
>       {
>         tree fns = lookup_name (fnname, LOOK_where::BLOCK_NAMESPACE);
> @@ -6423,11 +6429,6 @@ add_operator_candidates (z_candidate **candidates,
>   
>     if (!rewritten)
>       {
> -      /* The standard says to rewrite built-in candidates, too,
> -	 but there's no point.  */
> -      add_builtin_candidates (candidates, code, code2, fnname, arglist,
> -			      flags, complain);
> -
>         /* Maybe add C++20 rewritten comparison candidates.  */
>         tree_code rewrite_code = ERROR_MARK;
>         if (cxx_dialect >= cxx20
> diff --git a/gcc/testsuite/g++.dg/template/conv17.C b/gcc/testsuite/g++.dg/template/conv17.C
> index f0f10f2ef4f..87ecefb8de3 100644
> --- a/gcc/testsuite/g++.dg/template/conv17.C
> +++ b/gcc/testsuite/g++.dg/template/conv17.C
> @@ -61,3 +61,10 @@ concept E = requires { T().h(nullptr); };
>   
>   static_assert(!E<C>);
>   #endif
> +
> +// Verify that the strictly viable built-in operator+ candidate precludes
> +// us from computing all argument conversions for the below non-strictly
> +// viable non-member candidate.
> +enum N { n };
> +int operator+(N&, B);
> +int f = n + 42;
>