ICK_Function_Conversion is a third kind conversion

[aaronpuchert]

Summary

Not sure if this has any effect, but it was inconsistent before.

[aaronpuchert]

Remove wrong changes in SemaExprCXX.cpp.

[aaronpuchert]

Ping?

[aaronpuchert]

@rsmith Could you have a look at this?

[aaronpuchert]

Maybe this is to trivial for a review. The comment on StandardConversionSequence::Third in clang/Sema/Overload.h says

The third conversion can be a qualification conversion or a function conversion.

[aaron.ballman]

In D67113#2243451, @aaronpuchert wrote:

Maybe this is to trivial for a review. The comment on StandardConversionSequence::Third in clang/Sema/Overload.h says

The third conversion can be a qualification conversion or a function conversion.

There may be a deeper issue here in that there are four standard conversion sequences, not three: http://eel.is/c++draft/conv#1, but otherwise this change seems correct to me (function conversion is certainly not in the first group).

Do you have a test case which can exercise this change?

[aaronpuchert]

In D67113#2244463, @aaron.ballman wrote:

There may be a deeper issue here in that there are four standard conversion sequences, not three: http://eel.is/c++draft/conv#1, but otherwise this change seems correct to me (function conversion is certainly not in the first group).

It seems we've collapsed the third and fourth kind into one, I wonder if that makes a difference. This can even be observed in the AST:

void f() noexcept;
void (*const fp)() = f;
//     VarDecl fp 'void (*const)()' cinit
// (2) `-ImplicitCastExpr 'void (*const)()' <NoOp>
// (1)   `-ImplicitCastExpr 'void (*)() noexcept' <FunctionToPointerDecay>
//         `-DeclRefExpr 'void () noexcept' lvalue Function 'f' 'void () noexcept'

where (1) is a first kind conversion, and (2) a combined third and fourth kind conversion. Since both are essentially no-ops (see also D67112), it probably doesn't make sense to go into more detail.

Do you have a test case which can exercise this change?

Nope, just discovered this while reading the code. This should be unreachable, so it would be bad if I could observe a change.

clang/lib/Sema/SemaOverload.cpp

 /// is acceptable.
 static bool CheckConvertedConstantConversions(Sema &S,
                                               StandardConversionSequence &SCS) {
   // Since we know that the target type is an integral or unscoped enumeration
   // type, most conversion kinds are impossible. All possible First and Third
   // conversions are fine.
   switch (SCS.Second) {
   case ICK_Identity:
-  case ICK_Function_Conversion:
   case ICK_Integral_Promotion:
   case ICK_Integral_Conversion: // Narrowing conversions are checked elsewhere.
   case ICK_Zero_Queue_Conversion:
     return true;
 
   case ICK_Boolean_Conversion:
     // Conversion from an integral or unscoped enumeration type to bool is
     // classified as ICK_Boolean_Conversion, but it's also arguably an integral
static bool CheckConvertedConstantConversions(Sema &S,
   case ICK_Incompatible_Pointer_Conversion:
     return false;
 
   case ICK_Lvalue_To_Rvalue:
   case ICK_Array_To_Pointer:
   case ICK_Function_To_Pointer:
     llvm_unreachable("found a first conversion kind in Second");
 
+  case ICK_Function_Conversion:
   case ICK_Qualification:
     llvm_unreachable("found a third conversion kind in Second");
 
   case ICK_Num_Conversion_Kinds:
     break;
   }
 
   llvm_unreachable("unknown conversion kind");