[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 as per comments conserving the compat warnings!

Please if you could review.
If the changes look good, please if could commit the same for me.

Thanks,
Rahul


On Thu, Apr 17, 2014 at 11:15 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Thu, Apr 17, 2014 at 5:02 AM, Rahul Jain <1989.rahuljain at gmail.com>wrote:
>
>>
>> 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() {} };
>>
>
> This second comment change is now redundant; this is covered by 'might be
> void'.
>
>
>> +
>>        bool OK = getLangOpts().CPlusPlus1y &&
>> Dcl->getReturnType()->isVoidType();
>> +
>> +      if (!Dcl->getReturnType()->isVoidType() &&
>> +          !Dcl->getReturnType()->isDependentType()) {
>>
>
> Having an 'if' here is incorrect -- we should produce the diagnostic
> regardless, or we'll miss the C++11-compat warning (this should also be
> conditioned on C++1y mode). Please just add the dependent type check to
> 'OK' instead.
>
>
>>        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/20140419/232a96e9/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR18746_3.patch
Type: application/octet-stream
Size: 1685 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140419/232a96e9/attachment.obj>