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

[Richard Smith]

Committed with minor tweaks as r206929. Thanks!


On Sat, Apr 19, 2014 at 3:44 AM, Rahul Jain <1989.rahuljain at gmail.com>wrote:

>
> 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/20140422/ec78e0b8/attachment.html>