[Sema] Improve handling of function pointer conversions

[Mordante]

Summary

Starting with C++17 the noexcept is part of the function signature but a noexcept function can be converted to a noexcept(false) function. The overload resolution code handles it properly but expression builder doesn't. It causes the following code to trigger and assertion failure:

struct S {
    int f(void) noexcept { return 110; }
} s;

template <int(S::*a)(void)> int f10(void) { return (s.*a)(); }

int foo(void)
{
  return f10<&S::f >();
}

The fix adds an extra implicit cast when needed. I picked the CK_NoOp as CastKind since it seems to be the best fit. However I wonder whether it would be better to add a new value CK_FunctionPointerConversion.

This fixes bug 40024.

[Mordante]

Ping

[rsmith]

clang/lib/Sema/SemaTemplate.cpp

6996

Please capitalize local variable names to match the local convention.

clang/test/CXX/conv/conv.fctptr/template-noexcept-invalid.cpp

8

You can just say PR40024 instead of spelling out the complete URL. That'll also work better if we migrate our bug tracker to a different system one day. (Incidentally, llvm.org/PR40024 works too.)

22

Please use a -verify test for this instead of FileCheck, and combine the -valid and -invalid testcases into the same test file.

[Mordante]

Addresses the review remarks:

• resultTy -> ResultTy
• Move all unit tests in one file
• Change the unit test to use -verify instead of FileCheck
• Use PR40024 instead of link to Bugzilla

[rsmith]

LGTM, though I'd like to see this generalized to handle noreturn too.

clang/lib/Sema/SemaTemplate.cpp

6997

Can we remove the check for C++17 here? I would expect we want to consider the other kind of function conversion (from noreturn function to potentially-returning function) here too, and that applies in all language modes.

Testcase for that bug:

template<int (*)()> struct X {};
int f() __attribute__((noreturn));
X<&f> x;

[Mordante]

Removed the language version test.

I also added clang/test/CXX/conv/conv.fctptr/template-noreturn.cpp to show this is not enough to allow noreturn to all language versions, so this file should not be committed.

Do you want me to look at a way to allow the noreturn for C++98/C++11? If yes I prefer to make that a separate commit.

AFAIK the [[noreturn]] conversion is a clang extension, should I file a DR for the standard? AFAIK there isn't one yet.

If you still like the patch could you commit it (except clang/test/CXX/conv/conv.fctptr/template-noreturn.cpp) since I don't have commit access?

[Mordante]

@rsmith I now have commit access so I can commit the patch. Any suggestions regarding the previous remark?

clang/lib/Sema/SemaTemplate.cpp

if (ParamType->isMemberPointerType()) {
       // the element type on the parameter could be more qualified than the
       // element type in the expression we constructed.
       bool ObjCLifetimeConversion;
       if (IsQualificationConversion(((Expr*) RefExpr.get())->getType(),
                                     ParamType.getUnqualifiedType(), false,
                                     ObjCLifetimeConversion))
         RefExpr = ImpCastExprToType(RefExpr.get(), ParamType.getUnqualifiedType(), CK_NoOp);
 
+      // Starting with C++17 the noexcept is part of the function signature but
+      // a noexcept function can be converted to a noexcept(false) function.
+      QualType resultTy;
+      if (getLangOpts().CPlusPlus17 &&
+          IsFunctionConversion(((Expr *)RefExpr.get())->getType(),
+                               ParamType.getUnqualifiedType(), resultTy))
+        RefExpr = ImpCastExprToType(RefExpr.get(), resultTy, CK_NoOp);
+
       assert(!RefExpr.isInvalid() &&
              Context.hasSameType(((Expr*) RefExpr.get())->getType(),
                                  ParamType.getUnqualifiedType()));
       return RefExpr;
     }
   }
 
   QualType T = VD->getType().getNonReferenceType();

clang/test/CXX/conv/conv.fctptr/template-noexcept-invalid.cpp

+// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only %s
+// RUN: not %clang_cc1 -std=c++17 -fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -std=c++2a -fsyntax-only %s 2>&1 | FileCheck %s
+
+// Starting with C++17 the noexcept is part of the function signature but
+// a noexcept function can be converted to a noexcept(false) function.
+// The tests were added for https://bugs.llvm.org/show_bug.cgi?id=40024
+// Tests the no longer valid function pointer conversions
+
+struct S {
+  int f(void) { return 110; }
+} s;
+
+template <int (S::*a)(void) noexcept>
+int f10(void) { return (s.*a)(); }
+
+int foo(void) {
+  return f10<&S::f>();
+}
+
+// CHECK: error: no matching function for call to 'f10'

clang/test/CXX/conv/conv.fctptr/template-noexcept-valid.cpp

+// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only %s
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only %s
+
+// Starting with C++17 the noexcept is part of the function signature but
+// a noexcept function can be converted to a noexcept(false) function.
+// The tests were added for https://bugs.llvm.org/show_bug.cgi?id=40024
+// Tests the still valid function pointer conversions
+
+struct S {
+  int f(void) noexcept { return 110; }
+} s;
+
+template <int (S::*a)(void)>
+int f10(void) { return (s.*a)(); }
+
+int foo(void) {
+  return f10<&S::f>();
+}