Committed with minor tweaks as r206929. Thanks!
On Sat, Apr 19, 2014 at 3:44 AM, Rahul Jain <[email protected]>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 <[email protected]>wrote: > >> On Thu, Apr 17, 2014 at 5:02 AM, Rahul Jain <[email protected]>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 <[email protected]>wrote: >>> >>>> On Tue, Apr 8, 2014 at 10:56 AM, Rahul Jain >>>> <[email protected]>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 >>>>> >>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
