[Patch] Fix for PR18746 -rejects-valid on a constexpr member function of a class template with a deduced return type and no return statements

[Rahul Jain]

Hi Richard,

Thanks for your valuable inputs!
Updated patch to take care of the other case as well.
Added a test case for that too!


Index: lib/Sema/SemaDeclCXX.cpp
===================================================================
--- lib/Sema/SemaDeclCXX.cpp (revision 206450)
+++ lib/Sema/SemaDeclCXX.cpp (working copy)
@@ -1175,14 +1175,21 @@
   } else {
     if (ReturnStmts.empty()) {
       // C++1y doesn't require constexpr functions to contain a 'return'
-      // statement. We still do, unless the return type is void, because
+      // statement. We still do, unless the return type might be void,
because
       // otherwise if there's no return statement, the function cannot
-      // be used in a core constant expression.
+      // be used in a core constant expression. Also, deduced return type
+      // with no return statements are perfectly legal. For example:
+      // template<typename T> struct X { constexpr auto f() {} };
+
       bool OK = getLangOpts().CPlusPlus1y &&
Dcl->getReturnType()->isVoidType();
+
+      if (!Dcl->getReturnType()->isVoidType() &&
+          !Dcl->getReturnType()->isDependentType()) {
       Diag(Dcl->getLocation(),
            OK ? diag::warn_cxx11_compat_constexpr_body_no_return
               : diag::err_constexpr_body_no_return);
       return OK;
+      }
     }
     if (ReturnStmts.size() > 1) {
       Diag(ReturnStmts.back(),
Index: test/SemaCXX/cxx1y-deduced-return-type.cpp
===================================================================
--- test/SemaCXX/cxx1y-deduced-return-type.cpp (revision 206450)
+++ test/SemaCXX/cxx1y-deduced-return-type.cpp (working copy)
@@ -261,6 +261,8 @@

 namespace Constexpr {
   constexpr auto f1(int n) { return n; }
+  template<typename T> struct X { constexpr auto f() {} }; // PR18746
+  template<typename T> struct Y { constexpr T f() {} }; // ok
   struct NonLiteral { ~NonLiteral(); } nl; // expected-note
{{user-provided destructor}}
   constexpr auto f2(int n) { return nl; } // expected-error {{return type
'Constexpr::NonLiteral' is not a literal type}}
 }



Please if you could review the same!

Thanks,
Rahul


On Mon, Apr 14, 2014 at 6:51 AM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Tue, Apr 8, 2014 at 10:56 AM, Rahul Jain <1989.rahuljain at gmail.com>wrote:
>
>>
>> Hi Richard,
>>
>> Hi all,
>>
>> This patch fixes PR18746.
>>
>>
>> Index: lib/Sema/SemaDeclCXX.cpp
>> ===================================================================
>> --- lib/Sema/SemaDeclCXX.cpp (revision 205775)
>> +++ lib/Sema/SemaDeclCXX.cpp (working copy)
>> @@ -1177,12 +1177,19 @@
>>        // C++1y doesn't require constexpr functions to contain a 'return'
>>        // statement. We still do, unless the return type is void, because
>>        // otherwise if there's no return statement, the function cannot
>> -      // be used in a core constant expression.
>> +      // be used in a core constant expression. Also, deduced return type
>> +      // with no return statements are perfectly legal. For example:
>> +      // template<typename T> struct X { constexpr auto f() {} };
>> +
>>        bool OK = getLangOpts().CPlusPlus1y &&
>> Dcl->getReturnType()->isVoidType();
>> +
>> +      if (!getLangOpts().CPlusPlus1y ||
>> +          !Dcl->getReturnType()->getContainedAutoType()) {
>>
>
> I don't think this is quite the right test. We have the same issue here:
>
>  template<typename T> constexpr T f() {}
>
> So...
> 1) Update the comment to say 'unless the return type might be void'
> instead of 'unless the return type is void', and
> 2) Check for (Dcl->getReturnType()->isVoidType() ||
> Dcl->getReturnType()->isDependentType())
>
> At this point, we should have already replaced the 'auto' return type with
> a dependent 'auto' return type, so this check should handle both the
> template case and the 'auto' case.
>
>        Diag(Dcl->getLocation(),
>>             OK ? diag::warn_cxx11_compat_constexpr_body_no_return
>>                : diag::err_constexpr_body_no_return);
>>        return OK;
>> +      }
>>
>
> The `if` around this looks wrong. We want to provide the cxx11_compat
> diagnostic in all cases where the C++1y rule applies.
>
>
>>      }
>>      if (ReturnStmts.size() > 1) {
>>        Diag(ReturnStmts.back(),
>> Index: test/SemaCXX/cxx1y-deduced-return-type.cpp
>> ===================================================================
>> --- test/SemaCXX/cxx1y-deduced-return-type.cpp (revision 205775)
>> +++ test/SemaCXX/cxx1y-deduced-return-type.cpp (working copy)
>> @@ -261,6 +261,7 @@
>>
>>  namespace Constexpr {
>>    constexpr auto f1(int n) { return n; }
>> +  template<typename T> struct X { constexpr auto f() {} }; // PR18746
>>    struct NonLiteral { ~NonLiteral(); } nl; // expected-note
>> {{user-provided destructor}}
>>    constexpr auto f2(int n) { return nl; } // expected-error {{return
>> type 'Constexpr::NonLiteral' is not a literal type}}
>>  }
>>
>>
>> Please if someone could review the same.
>>
>> Thanks,
>> Rahul
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140417/52832f04/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR18746_2.patch
Type: application/octet-stream
Size: 1947 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140417/52832f04/attachment.obj>