This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Improve handling of function pointer conversions
AcceptedPublic

Authored by Mordante on Jul 17 2019, 11:16 AM.

Details

Reviewers
rsmith
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.

Diff Detail

Event Timeline

Mordante created this revision.Jul 17 2019, 11:16 AM
lebedev.ri retitled this revision from Improve handling of function pointer conversions to [Sema] Improve handling of function pointer conversions.Aug 17 2019, 6:04 AM
rsmith added inline comments.Aug 29 2019, 1:28 PM
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 ↗(On Diff #210369)

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 ↗(On Diff #210369)

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

Mordante updated this revision to Diff 218215.Aug 31 2019, 5:20 AM

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
Mordante marked an inline comment as done.Aug 31 2019, 5:20 AM
rsmith accepted this revision.Oct 4 2019, 11:11 AM

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;
This revision is now accepted and ready to land.Oct 4 2019, 11:11 AM
Mordante updated this revision to Diff 224753.Oct 12 2019, 10:11 AM

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 marked an inline comment as done.Oct 12 2019, 10:11 AM

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