From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 98191 invoked by alias); 1 Mar 2018 21:57:07 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 97465 invoked by uid 89); 1 Mar 2018 21:57:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,HTML_MESSAGE,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy= X-HELO: mail-it0-f45.google.com Received: from mail-it0-f45.google.com (HELO mail-it0-f45.google.com) (209.85.214.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 01 Mar 2018 21:57:03 +0000 Received: by mail-it0-f45.google.com with SMTP id n7so9365130ita.5 for ; Thu, 01 Mar 2018 13:57:03 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=UGIEJD7hBikA2pNPKD7oeOq1AV4C2GOEGSi5XWmEdIQ=; b=Q1ftTRBjJ0keSV8LQibUKIeXIIYBGKe3x9eqLUGDgF2xebqTB9yl5l4ehc/GrGRgIb fEcaH0YhS40fL9ZjeEgxHLcYeSyfwMfcqQ/k28aYVXaV2TaEd7Rh9PxBfXcu8vtnpc3q t4SbUjUWaE2gxT5z/a/+zppMBTEWmCNoT3DRsZjXo+E9UYW2yOlUj7jTlgtODfTgaIsr r+ZmQ0k+OC5ifH75sFMgaSJ8eNl0Kvug9IZIEQARH8+U7GJN5bKGC37/KM138sY3hqmx 5R2PbEjSg95D4fJWhaS2kMPK6nwIt3l0z2P/MqQgMjyaMBi9WYYpRqciP0gHC/gsvQeN T+zQ== X-Gm-Message-State: APf1xPB3gDaQAmU4R+WTo05OLCnWCuIckpoe1n7C7acjCRGqtjcioTdD rhlr1qf4Ud4ESHgOb59EKiczrniUU8g/vqYaJa086g== X-Google-Smtp-Source: AG47ELs+BRBS5LpyCyVBq/npq6m6SrjC5WsprjsVOvqAidJf63FYnSAOiJl0zfa0J6GRlJMcgmy0O/IRe6LUGc5ru2c= X-Received: by 10.36.36.144 with SMTP id f138mr4281182ita.131.1519941421420; Thu, 01 Mar 2018 13:57:01 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.17.200 with HTTP; Thu, 1 Mar 2018 13:57:00 -0800 (PST) Received: by 10.107.17.200 with HTTP; Thu, 1 Mar 2018 13:57:00 -0800 (PST) In-Reply-To: <20180301214004.GG16833@redhat.com> References: <20180227191313.GN2995@redhat.com> <63560c5d-83e0-3ba5-123f-787b575577ef@redhat.com> <20180228143243.GP2995@redhat.com> <20180228211948.GA11663@redhat.com> <20180301131725.GB16833@redhat.com> <20180301214004.GG16833@redhat.com> From: Jason Merrill Date: Thu, 01 Mar 2018 21:57:00 -0000 Message-ID: Subject: Re: C++ PATCH to fix static init with () in a template (PR c++/84582) To: Marek Polacek Cc: gcc-patches List Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-03/txt/msg00066.txt.bz2 Ok. On Mar 1, 2018 4:40 PM, "Marek Polacek" wrote: > On Thu, Mar 01, 2018 at 01:56:50PM -0500, Jason Merrill wrote: > > On Thu, Mar 1, 2018 at 8:17 AM, Marek Polacek > wrote: > > > On Wed, Feb 28, 2018 at 04:50:39PM -0500, Jason Merrill wrote: > > >> On Wed, Feb 28, 2018 at 4:19 PM, Marek Polacek > wrote: > > >> > On Wed, Feb 28, 2018 at 10:51:17AM -0500, Jason Merrill wrote: > > >> >> On Wed, Feb 28, 2018 at 9:32 AM, Marek Polacek > wrote: > > >> >> > On Tue, Feb 27, 2018 at 04:16:31PM -0500, Jason Merrill wrote: > > >> >> >> On 02/27/2018 02:13 PM, Marek Polacek wrote: > > >> >> >> > My recent change introducing cxx_constant_init caused this > code > > >> >> >> > > > >> >> >> > template class A { > > >> >> >> > static const long b = 0; > > >> >> >> > static const unsigned c = (b); > > >> >> >> > }; > > >> >> >> > > > >> >> >> > to be rejected. The reason is that force_paren_expr turns > "b" into "*(const > > >> >> >> > long int &) &b", where the former is not value-dependent but > the latter is > > >> >> >> > value-dependent. So when we get to maybe_constant_init_1: > > >> >> >> > 5147 if (!is_nondependent_static_init_expression (t)) > > >> >> >> > 5148 /* Don't try to evaluate it. */; > > >> >> >> > it's not evaluated and we get the non-constant initialization > error. > > >> >> >> > (Before we'd always evaluated the expression.) > > >> >> >> > > > >> >> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > >> >> >> > > > >> >> >> > 2018-02-27 Marek Polacek > > >> >> >> > > > >> >> >> > PR c++/84582 > > >> >> >> > * semantics.c (force_paren_expr): Avoid creating a static > cast > > >> >> >> > when processing a template. > > >> >> >> > > > >> >> >> > * g++.dg/cpp1z/static1.C: New test. > > >> >> >> > * g++.dg/template/static37.C: New test. > > >> >> >> > > > >> >> >> > diff --git gcc/cp/semantics.c gcc/cp/semantics.c > > >> >> >> > index 35569d0cb0d..b48de2df4e2 100644 > > >> >> >> > --- gcc/cp/semantics.c > > >> >> >> > +++ gcc/cp/semantics.c > > >> >> >> > @@ -1697,7 +1697,7 @@ force_paren_expr (tree expr) > > >> >> >> > expr = build1 (PAREN_EXPR, TREE_TYPE (expr), expr); > > >> >> >> > else if (VAR_P (expr) && DECL_HARD_REGISTER (expr)) > > >> >> >> > /* We can't bind a hard register variable to a > reference. */; > > >> >> >> > - else > > >> >> >> > + else if (!processing_template_decl) > > >> >> >> > > >> >> >> Hmm, this means that we forget about the parentheses in a > template. I'm > > >> >> >> surprised that this didn't break anything in the testsuite. In > particular, > > >> >> >> auto-fn15.C. I've attached an addition to auto-fn15.C to catch > this issue. > > >> >> > > > >> >> > Thanks, you're right. I'll use it. > > >> >> > > > >> >> >> Can we use PAREN_EXPR instead of the static_cast in a template? > > >> >> > > > >> >> > I don't think so, it would fix the issue you pointed out in > auto-fn15.C but > > >> >> > it wouldn't fix the original test. The problem with using > PAREN_EXPR in a > > >> >> > template is that instantiate_non_dependent_expr will turn in > into the > > >> >> > static cast anyway; tsubst_copy_and_build has > > >> >> > case PAREN_EXPR: > > >> >> > RETURN (finish_parenthesized_expr (RECUR (TREE_OPERAND (t, > 0)))); > > >> >> > so it calls force_paren_expr and this time we're not in a > template. And > > >> >> > then when calling cxx_constant_init we have the same issue. > > >> >> > > >> >> Then maybe we need something like fold_non_dependent_expr, which > > >> >> checks for dependency before substitution and then immediately > > >> >> evaluates the result. > > >> > > > >> > I hope you meant something like this. Further testing also > revealed that > > >> > maybe_undo_parenthesized_ref should be able to unwrap PAREN_EXPR > (so that > > >> > (fn1)(); in paren2.C is handled correctly), and that lvalue_kind > should look > > >> > into PAREN_EXPR so as to give the correct answer regarding > lvalueness: we > > >> > should accept > > >> > > > >> > template > > >> > void foo (int i) > > >> > { > > >> > ++(i); > > >> > } > > >> > > > >> > Apologies if I'm on the wrong track. > > >> > > > >> > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > >> > > > >> > 2018-02-28 Marek Polacek > > >> > Jason Merrill > > >> > > > >> > PR c++/84582 > > >> > * semantics.c (force_paren_expr): Avoid creating the static > cast > > >> > when in a template. Create a PAREN_EXPR when in a template. > > >> > (maybe_undo_parenthesized_ref): Unwrap PAREN_EXPR. > > >> > * typeck2.c (store_init_value): Call > fold_non_dependent_expr instead > > >> > of instantiate_non_dependent_expr. > > >> > * tree.c (lvalue_kind): Handle PAREN_EXPR like > NON_DEPENDENT_EXPR. > > >> > > > >> > * g++.dg/cpp1y/auto-fn15.C: Extend testing. > > >> > * g++.dg/cpp1z/static1.C: New test. > > >> > * g++.dg/template/static37.C: New test. > > >> > > > >> > diff --git gcc/cp/semantics.c gcc/cp/semantics.c > > >> > index 35569d0cb0d..722e3718a14 100644 > > >> > --- gcc/cp/semantics.c > > >> > +++ gcc/cp/semantics.c > > >> > @@ -1697,7 +1697,7 @@ force_paren_expr (tree expr) > > >> > expr = build1 (PAREN_EXPR, TREE_TYPE (expr), expr); > > >> > else if (VAR_P (expr) && DECL_HARD_REGISTER (expr)) > > >> > /* We can't bind a hard register variable to a reference. */; > > >> > - else > > >> > + else if (!processing_template_decl) > > >> > { > > >> > cp_lvalue_kind kind = lvalue_kind (expr); > > >> > if ((kind & ~clk_class) != clk_none) > > >> > @@ -1713,6 +1713,8 @@ force_paren_expr (tree expr) > > >> > REF_PARENTHESIZED_P (expr) = true; > > >> > } > > >> > } > > >> > + else > > >> > + expr = build1 (PAREN_EXPR, TREE_TYPE (expr), expr); > > >> > > >> There's already a branch for building PAREN_EXPR, let's just replace > > >> its condition. > > > > > > Sure. > > > > > >> > - value = instantiate_non_dependent_expr (value); > > >> > + value = fold_non_dependent_expr (value); > > >> > > >> I was thinking that we want a parallel fold_non_dependent_init (that > > >> hopefully shares most of the implementation). Then we shouldn't need > > >> the call to maybe_constant_init anymore. > > > > > > If you mean fold_non_dependent_init that would be like > fold_non_dependent_expr > > > but with maybe_constant_init and not maybe_constant_value > > > > And is_nondependent_static_init_expression, and different arguments to > > cxx_eval_outermost_constant_expression, yes. > > Ah. Maybe it'll be useful sometime in the future. > > > > then that would break e.g. > > > > > > const double d = 9.0; // missing constexpr > > > constexpr double j = d; // should give error > > > > > > because maybe_constant_value checks is_nondependent_constant_expression, > and > > > "d" in the example above is not a constant expression, so we don't > evaluate, > > > and "d" stays "d", so require_constant_expression gives the error. On > the > > > other hand, maybe_constant_init checks is_nondependent_static_init_ > expression, > > > and "d" is that, so we evaluate "d" to "9.0". Then > require_constant_expression > > > doesn't complain. > > > > Ah, I see. You're right, let's stick with fold_non_dependent_expr. > > Thanks, so this is the final patch then: > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2018-03-01 Marek Polacek > Jason Merrill > > PR c++/84582 > * semantics.c (force_paren_expr): Create a PAREN_EXPR when in > a template. > (maybe_undo_parenthesized_ref): Unwrap PAREN_EXPR. > * typeck2.c (store_init_value): Call fold_non_dependent_expr > instead > of instantiate_non_dependent_expr. > * tree.c (lvalue_kind): Handle PAREN_EXPR like NON_DEPENDENT_EXPR. > > * g++.dg/cpp1y/auto-fn15.C: Extend testing. > * g++.dg/cpp1z/static1.C: New test. > * g++.dg/template/static37.C: New test. > > diff --git gcc/cp/semantics.c gcc/cp/semantics.c > index 87c5c669a55..1ac1d23e761 100644 > --- gcc/cp/semantics.c > +++ gcc/cp/semantics.c > @@ -1693,7 +1693,8 @@ force_paren_expr (tree expr) > if (TREE_CODE (expr) == COMPONENT_REF > || TREE_CODE (expr) == SCOPE_REF) > REF_PARENTHESIZED_P (expr) = true; > - else if (type_dependent_expression_p (expr)) > + else if (type_dependent_expression_p (expr) > + || processing_template_decl) > expr = build1 (PAREN_EXPR, TREE_TYPE (expr), expr); > else if (VAR_P (expr) && DECL_HARD_REGISTER (expr)) > /* We can't bind a hard register variable to a reference. */; > @@ -1724,9 +1725,10 @@ force_paren_expr (tree expr) > tree > maybe_undo_parenthesized_ref (tree t) > { > - if (cxx_dialect >= cxx14 > - && INDIRECT_REF_P (t) > - && REF_PARENTHESIZED_P (t)) > + if (cxx_dialect < cxx14) > + return t; > + > + if (INDIRECT_REF_P (t) && REF_PARENTHESIZED_P (t)) > { > t = TREE_OPERAND (t, 0); > while (TREE_CODE (t) == NON_LVALUE_EXPR > @@ -1737,6 +1739,8 @@ maybe_undo_parenthesized_ref (tree t) > || TREE_CODE (t) == STATIC_CAST_EXPR); > t = TREE_OPERAND (t, 0); > } > + else if (TREE_CODE (t) == PAREN_EXPR) > + t = TREE_OPERAND (t, 0); > > return t; > } > diff --git gcc/cp/tree.c gcc/cp/tree.c > index 9b9e36a1173..19f1c0629c9 100644 > --- gcc/cp/tree.c > +++ gcc/cp/tree.c > @@ -239,6 +239,7 @@ lvalue_kind (const_tree ref) > return lvalue_kind (BASELINK_FUNCTIONS (CONST_CAST_TREE (ref))); > > case NON_DEPENDENT_EXPR: > + case PAREN_EXPR: > return lvalue_kind (TREE_OPERAND (ref, 0)); > > case VIEW_CONVERT_EXPR: > diff --git gcc/cp/typeck2.c gcc/cp/typeck2.c > index 153b46cca77..583c65d4d0a 100644 > --- gcc/cp/typeck2.c > +++ gcc/cp/typeck2.c > @@ -822,7 +822,7 @@ store_init_value (tree decl, tree init, vec va_gc>** cleanups, int flags) > if (decl_maybe_constant_var_p (decl) || TREE_STATIC (decl)) > { > bool const_init; > - value = instantiate_non_dependent_expr (value); > + value = fold_non_dependent_expr (value); > if (DECL_DECLARED_CONSTEXPR_P (decl) > || (DECL_IN_AGGR_P (decl) && !DECL_VAR_DECLARED_INLINE_P (decl))) > { > diff --git gcc/testsuite/g++.dg/cpp1y/auto-fn15.C > gcc/testsuite/g++.dg/cpp1y/auto-fn15.C > index ba9f3579f62..0db428f7270 100644 > --- gcc/testsuite/g++.dg/cpp1y/auto-fn15.C > +++ gcc/testsuite/g++.dg/cpp1y/auto-fn15.C > @@ -22,6 +22,8 @@ template > decltype(auto) h5(T t) { return t.i; } > template > decltype(auto) h6(T t) { return (t.i); } > +template > +decltype(auto) h7(T t) { return (i); } > > int main() > { > @@ -48,4 +50,5 @@ int main() > same_type(); > same_type(); > same_type(); > + same_type(); > } > diff --git gcc/testsuite/g++.dg/cpp1z/static1.C > gcc/testsuite/g++.dg/cpp1z/static1.C > index e69de29bb2d..cb872997c5a 100644 > --- gcc/testsuite/g++.dg/cpp1z/static1.C > +++ gcc/testsuite/g++.dg/cpp1z/static1.C > @@ -0,0 +1,19 @@ > +// PR c++/84582 > +// { dg-options -std=c++17 } > + > +class C { > + static inline const long b = 0; > + static inline const unsigned c = (b); > +}; > +class D { > + static inline const long b = 0; > + static inline const unsigned c = b; > +}; > +template class A { > + static inline const long b = 0; > + static inline const unsigned c = (b); > +}; > +template class B { > + static inline const long b = 0; > + static inline const unsigned c = b; > +}; > diff --git gcc/testsuite/g++.dg/template/static37.C > gcc/testsuite/g++.dg/template/static37.C > index e69de29bb2d..90bc65d2fbc 100644 > --- gcc/testsuite/g++.dg/template/static37.C > +++ gcc/testsuite/g++.dg/template/static37.C > @@ -0,0 +1,18 @@ > +// PR c++/84582 > + > +class C { > + static const long b = 0; > + static const unsigned c = (b); > +}; > +class D { > + static const long b = 0; > + static const unsigned c = b; > +}; > +template class A { > + static const long b = 0; > + static const unsigned c = (b); > +}; > +template class B { > + static const long b = 0; > + static const unsigned c = b; > +}; > > Marek >