[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]

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/20140417/51376124/attachment.html>