[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 a lot for your help on this.
Highly appreciate it!

I have closed the bug.


Thanks to Suyog Sarda for helping me out on this!

Thanks,
Rahul


On Wed, Apr 23, 2014 at 4:51 AM, Richard Smith <richard at metafoo.co.uk>wrote:

> 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/20140423/17cc4b04/attachment.html>