From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67511 invoked by alias); 25 May 2017 19:02:44 -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 67496 invoked by uid 89); 25 May 2017 19:02:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy= X-HELO: mail-io0-f179.google.com Received: from mail-io0-f179.google.com (HELO mail-io0-f179.google.com) (209.85.223.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 25 May 2017 19:02:41 +0000 Received: by mail-io0-f179.google.com with SMTP id f102so141710031ioi.2 for ; Thu, 25 May 2017 12:02:44 -0700 (PDT) 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=qOO0QAKcHfbttFae3xpRjnugW6BQrHB3eUCnWCdWIps=; b=IDgBpwOSl3P4QYGgmHT0IDd1hv4w/rvg5CiirYXiHHUE+V+f6q8uuO0k+FYty6EsPL xtcEOt7+nCP5SHt7nriXvVO6sorYOpvAuCudhJr9Z/ePma5aKRNYoGFLJzt0Gx4eLgfU 0bfEboW8bZSBXxXhMWLPtvtR/Gpvo/59McMqz8GWO4wK/+LYDDM0rbHjBO59cA7mIoHg t6LCqyTK/vFAMD4Db816tA6hlii6uoE/6l95xaAqKNuGNvfciA6NIfNWnpfy0DFgCKPU l9KSORhd9l6S74hONq80lymbIn5rItHWHLjE4B+Y9E+t79Iac8Dk64PsGVL+H/+cStYv HxUQ== X-Gm-Message-State: AODbwcCBkmZEZcT8xNC3LhNGa0l2uky48/Q3eNjQHn4ZHOb1p9Dq6HdT Owu+cEO5nHQ74XghM9NXNP3u2cTviB+J X-Received: by 10.107.6.218 with SMTP id f87mr38388625ioi.2.1495738962847; Thu, 25 May 2017 12:02:42 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.6.226 with HTTP; Thu, 25 May 2017 12:02:22 -0700 (PDT) In-Reply-To: <20170525142232.GF8499@tucnak> References: <20170524064853.GY8499@tucnak> <20170525142232.GF8499@tucnak> From: Jason Merrill Date: Thu, 25 May 2017 19:08:00 -0000 Message-ID: Subject: Re: [C++ PATCH] Fix -Wunused with C++17 structured bindings To: Jakub Jelinek Cc: gcc-patches List Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg01982.txt.bz2 OK, thanks. On Thu, May 25, 2017 at 10:22 AM, Jakub Jelinek wrote: > On Wed, May 24, 2017 at 12:13:40PM -0400, Jason Merrill wrote: >> On Wed, May 24, 2017 at 2:48 AM, Jakub Jelinek wrote: >> > On Tue, May 23, 2017 at 09:45:10PM -0400, Jason Merrill wrote: >> >> Someone on IRC complained that there was no way to suppress -Wunused >> >> on structured bindings. It seemed to me that the way the feature >> >> works, we shouldn't warn about the bindings individually; users need >> >> to give each of the subobjects a name even if they're only interested >> >> in using one of them. >> >> >> >> So this patch switches to tracking whether the underlying aggregate >> >> object as a whole is used; using one of the bindings will avoid any >> >> warning. >> >> >> >> This doesn't apply to tuple structured bindings, since in that case >> >> the bindings are actual variables rather than aliases to subobjects. >> > >> > So, shall we have even for the tuple structured bindings somewhere (in >> > lang_decl, such as adding struct lang_decl_decomp that would include >> > lang_decl_min?) the tree for the underlying artificial var decl? >> >> That would make sense. > > So like this (so far lightly tested)? As DECL_DECOMPOSITION_P bit > sits in DECL_LANG_SPECIFIC, it is not possible to set that bit first and > then retrofit_lang_decl, so I had to play some games with that. > > There is one issue I've noticed in your earlier patch (fixed in this patch), > we have separate DECL_HAS_VALUE_EXPR_P bit and DECL_VALUE_EXPR, testing > the former is cheap, the latter is an expensive function call. > So > if (DECL_VALUE_EXPR (exp)) > mark_exp_read (DECL_VALUE_EXPR (exp)); > is something unnecessarily expensive for all vars, while > if (DECL_HAS_VALUE_EXPR_P (exp)) > mark_exp_read (DECL_VALUE_EXPR (exp)); > or > if (DECL_HAS_VALUE_EXPR_P (exp)) > { > tree t = DECL_VALUE_EXPR (exp); > if (t) > mark_exp_read (t); > } > is much cheaper. I believe usually DECL_VALUE_EXPR should be non-NULL > if DECL_HAS_VALUE_EXPR_P, but am not 100% sure if it is always guaranteed > (but pretty sure lots of code would break if it is not). > > 2017-05-25 Jakub Jelinek > > * cp-tree.h (struct lang_decl_decomp): New type. > (struct lang_decl): Add u.decomp. > (LANG_DECL_DECOMP_CHECK): Define. > (DECL_DECOMPOSITION_P): Note it is set also on the vars > for user identifiers. > (DECL_DECOMP_BASE): Define. > (retrofit_lang_decl): Add extra int = 0 argument. > * lex.c (retrofit_lang_decl): Add SEL argument, if non-zero > use it to influence the selector choices and for selector > 0 to non-zero transition copy old content. > (cxx_dup_lang_specific_decl): Handle DECL_DECOMPOSITION_P. > * decl.c (poplevel): For DECL_DECOMPOSITION_P, check > !DECL_DECOMP_BASE instead of !DECL_VALUE_EXPR. Adjust warning > wording if decl is a structured binding. > (cp_finish_decomp): Pass 4 as the new argument to retrofit_lang_decl. > Set DECL_DECOMP_BASE. Ignore DECL_READ_P sets from initialization > of individual variables for tuple structured bindings. > (grokdeclarator): Pass 4 as the new argument to retrofit_lang_decl. > Clear DECL_DECOMP_BASE. > * decl2.c (mark_used): Mark DECL_DECOMP_BASE TREE_USED as well. > * pt.c (tsubst_decomp_names): Assert DECL_DECOMP_BASE matches what > is expected. > * expr.c (mark_exp_read): Recurse on DECL_DECOMP_BASE instead of > DECL_VALUE_EXPR. > > * g++.dg/cpp1z/decomp29.C (p): New variable. > (main): Add further tests. > > --- gcc/cp/cp-tree.h.jj 2017-05-25 10:37:00.000000000 +0200 > +++ gcc/cp/cp-tree.h 2017-05-25 12:48:34.828167911 +0200 > @@ -2516,6 +2516,15 @@ struct GTY(()) lang_decl_parm { > int index; > }; > > +/* Additional DECL_LANG_SPECIFIC information for structured bindings. */ > + > +struct GTY(()) lang_decl_decomp { > + struct lang_decl_min min; > + /* The artificial underlying "e" variable of the structured binding > + variable. */ > + tree base; > +}; > + > /* DECL_LANG_SPECIFIC for all types. It would be nice to just make this a > union rather than a struct containing a union as its only field, but > tree.h declares it as a struct. */ > @@ -2527,6 +2536,7 @@ struct GTY(()) lang_decl { > struct lang_decl_fn GTY ((tag ("1"))) fn; > struct lang_decl_ns GTY((tag ("2"))) ns; > struct lang_decl_parm GTY((tag ("3"))) parm; > + struct lang_decl_decomp GTY((tag ("4"))) decomp; > } u; > }; > > @@ -2563,6 +2573,13 @@ struct GTY(()) lang_decl { > lang_check_failed (__FILE__, __LINE__, __FUNCTION__); \ > <->u.parm; }) > > +#define LANG_DECL_DECOMP_CHECK(NODE) __extension__ \ > +({ struct lang_decl *lt = DECL_LANG_SPECIFIC (NODE); \ > + if (!VAR_P (NODE) \ > + || lt->u.base.selector != 4) \ > + lang_check_failed (__FILE__, __LINE__, __FUNCTION__); \ > + <->u.decomp; }) > + > #define LANG_DECL_U2_CHECK(NODE, TF) __extension__ \ > ({ struct lang_decl *lt = DECL_LANG_SPECIFIC (NODE); \ > if (!LANG_DECL_HAS_MIN (NODE) || lt->u.base.u2sel != TF) \ > @@ -2583,6 +2600,9 @@ struct GTY(()) lang_decl { > #define LANG_DECL_PARM_CHECK(NODE) \ > (&DECL_LANG_SPECIFIC (NODE)->u.parm) > > +#define LANG_DECL_DECOMP_CHECK(NODE) \ > + (&DECL_LANG_SPECIFIC (NODE)->u.decomp) > + > #define LANG_DECL_U2_CHECK(NODE, TF) \ > (&DECL_LANG_SPECIFIC (NODE)->u.min.u2) > > @@ -3829,8 +3849,8 @@ more_aggr_init_expr_args_p (const aggr_i > (DECL_LANG_SPECIFIC (VAR_DECL_CHECK (NODE))->u.base.var_declared_inline_p \ > = true) > > -/* Nonzero if NODE is an artificial VAR_DECL for a C++17 decomposition > - declaration. */ > +/* Nonzero if NODE is an artificial VAR_DECL for a C++17 structured binding > + declaration or one of VAR_DECLs for the user identifiers in it. */ > #define DECL_DECOMPOSITION_P(NODE) \ > (VAR_P (NODE) && DECL_LANG_SPECIFIC (NODE) \ > ? DECL_LANG_SPECIFIC (NODE)->u.base.decomposition_p \ > @@ -3839,6 +3859,10 @@ more_aggr_init_expr_args_p (const aggr_i > (DECL_LANG_SPECIFIC (VAR_DECL_CHECK (NODE))->u.base.decomposition_p \ > = true) > > +/* The underlying artificial VAR_DECL for structured binding. */ > +#define DECL_DECOMP_BASE(NODE) \ > + (LANG_DECL_DECOMP_CHECK (NODE)->base) > + > /* Nonzero if NODE is an inline VAR_DECL. In C++17, static data members > declared with constexpr specifier are implicitly inline variables. */ > #define DECL_INLINE_VAR_P(NODE) \ > @@ -6274,7 +6298,7 @@ extern tree unqualified_name_lookup_erro > extern tree unqualified_fn_lookup_error (cp_expr); > extern tree build_lang_decl (enum tree_code, tree, tree); > extern tree build_lang_decl_loc (location_t, enum tree_code, tree, tree); > -extern void retrofit_lang_decl (tree); > +extern void retrofit_lang_decl (tree, int = 0); > extern tree copy_decl (tree CXX_MEM_STAT_INFO); > extern tree copy_type (tree CXX_MEM_STAT_INFO); > extern tree cxx_make_type (enum tree_code); > --- gcc/cp/lex.c.jj 2017-05-11 09:42:27.000000000 +0200 > +++ gcc/cp/lex.c 2017-05-25 15:01:01.362356364 +0200 > @@ -529,16 +529,28 @@ build_lang_decl_loc (location_t loc, enu > and pushdecl (for functions generated by the back end). */ > > void > -retrofit_lang_decl (tree t) > +retrofit_lang_decl (tree t, int sel) > { > struct lang_decl *ld; > size_t size; > - int sel; > + size_t oldsize = 0; > > if (DECL_LANG_SPECIFIC (t)) > - return; > + { > + if (sel) > + { > + if (DECL_LANG_SPECIFIC (t)->u.base.selector == sel) > + return; > + gcc_assert (DECL_LANG_SPECIFIC (t)->u.base.selector == 0); > + oldsize = sizeof (struct lang_decl_min); > + } > + else > + return; > + } > > - if (TREE_CODE (t) == FUNCTION_DECL) > + if (sel == 4) > + size = sizeof (struct lang_decl_decomp); > + else if (TREE_CODE (t) == FUNCTION_DECL) > sel = 1, size = sizeof (struct lang_decl_fn); > else if (TREE_CODE (t) == NAMESPACE_DECL) > sel = 2, size = sizeof (struct lang_decl_ns); > @@ -550,6 +562,8 @@ retrofit_lang_decl (tree t) > gcc_unreachable (); > > ld = (struct lang_decl *) ggc_internal_cleared_alloc (size); > + if (oldsize) > + memcpy (ld, DECL_LANG_SPECIFIC (t), oldsize); > > ld->u.base.selector = sel; > > @@ -584,6 +598,8 @@ cxx_dup_lang_specific_decl (tree node) > size = sizeof (struct lang_decl_ns); > else if (TREE_CODE (node) == PARM_DECL) > size = sizeof (struct lang_decl_parm); > + else if (DECL_DECOMPOSITION_P (node)) > + size = sizeof (struct lang_decl_decomp); > else if (LANG_DECL_HAS_MIN (node)) > size = sizeof (struct lang_decl_min); > else > --- gcc/cp/decl.c.jj 2017-05-25 10:37:00.000000000 +0200 > +++ gcc/cp/decl.c 2017-05-25 14:35:54.600274460 +0200 > @@ -658,7 +658,7 @@ poplevel (int keep, int reverse, int fun > && ! DECL_IN_SYSTEM_HEADER (decl) > /* For structured bindings, consider only real variables, not > subobjects. */ > - && (DECL_DECOMPOSITION_P (decl) ? !DECL_VALUE_EXPR (decl) > + && (DECL_DECOMPOSITION_P (decl) ? !DECL_DECOMP_BASE (decl) > : (DECL_NAME (decl) && !DECL_ARTIFICIAL (decl))) > && type != error_mark_node > && (!CLASS_TYPE_P (type) > @@ -667,16 +667,28 @@ poplevel (int keep, int reverse, int fun > TYPE_ATTRIBUTES (TREE_TYPE (decl))))) > { > if (! TREE_USED (decl)) > - warning_at (DECL_SOURCE_LOCATION (decl), > - OPT_Wunused_variable, "unused variable %qD", decl); > + { > + if (!DECL_NAME (decl) && DECL_DECOMPOSITION_P (decl)) > + warning_at (DECL_SOURCE_LOCATION (decl), > + OPT_Wunused_variable, > + "unused structured binding declaration"); > + else > + warning_at (DECL_SOURCE_LOCATION (decl), > + OPT_Wunused_variable, "unused variable %qD", decl); > + } > else if (DECL_CONTEXT (decl) == current_function_decl > // For -Wunused-but-set-variable leave references alone. > && TREE_CODE (TREE_TYPE (decl)) != REFERENCE_TYPE > && errorcount == unused_but_set_errorcount) > { > - warning_at (DECL_SOURCE_LOCATION (decl), > - OPT_Wunused_but_set_variable, > - "variable %qD set but not used", decl); > + if (!DECL_NAME (decl) && DECL_DECOMPOSITION_P (decl)) > + warning_at (DECL_SOURCE_LOCATION (decl), > + OPT_Wunused_but_set_variable, "structured " > + "binding declaration set but not used"); > + else > + warning_at (DECL_SOURCE_LOCATION (decl), > + OPT_Wunused_but_set_variable, > + "variable %qD set but not used", decl); > unused_but_set_errorcount = errorcount; > } > } > @@ -7361,8 +7373,9 @@ cp_finish_decomp (tree decl, tree first, > } > if (processing_template_decl) > { > - retrofit_lang_decl (first); > + retrofit_lang_decl (first, 4); > SET_DECL_DECOMPOSITION_P (first); > + DECL_DECOMP_BASE (first) = decl; > } > first = DECL_CHAIN (first); > } > @@ -7375,8 +7388,9 @@ cp_finish_decomp (tree decl, tree first, > for (unsigned int i = 0; i < count; i++, d = DECL_CHAIN (d)) > { > v[count - i - 1] = d; > - retrofit_lang_decl (d); > + retrofit_lang_decl (d, 4); > SET_DECL_DECOMPOSITION_P (d); > + DECL_DECOMP_BASE (d) = decl; > } > > tree type = TREE_TYPE (decl); > @@ -7482,6 +7496,7 @@ cp_finish_decomp (tree decl, tree first, > eltscnt = tree_to_uhwi (tsize); > if (count != eltscnt) > goto cnt_mismatch; > + int save_read = DECL_READ_P (decl); > for (unsigned i = 0; i < count; ++i) > { > location_t sloc = input_location; > @@ -7514,6 +7529,10 @@ cp_finish_decomp (tree decl, tree first, > cp_finish_decl (v[i], init, /*constexpr*/false, > /*asm*/NULL_TREE, LOOKUP_NORMAL); > } > + /* Ignore reads from the underlying decl performed during initialization > + of the individual variables. If those will be read, we'll mark > + the underlying decl as read at that point. */ > + DECL_READ_P (decl) = save_read; > } > else if (TREE_CODE (type) == UNION_TYPE) > { > @@ -12295,9 +12314,10 @@ grokdeclarator (const cp_declarator *dec > { > gcc_assert (declarator && declarator->kind == cdk_decomp); > DECL_SOURCE_LOCATION (decl) = declarator->id_loc; > - retrofit_lang_decl (decl); > + retrofit_lang_decl (decl, 4); > DECL_ARTIFICIAL (decl) = 1; > SET_DECL_DECOMPOSITION_P (decl); > + DECL_DECOMP_BASE (decl) = NULL_TREE; > } > } > > --- gcc/cp/decl2.c.jj 2017-05-24 11:59:02.000000000 +0200 > +++ gcc/cp/decl2.c 2017-05-25 14:43:01.216917214 +0200 > @@ -5027,6 +5027,9 @@ mark_used (tree decl, tsubst_flags_t com > > /* Set TREE_USED for the benefit of -Wunused. */ > TREE_USED (decl) = 1; > + /* And for structured bindings also the underlying decl. */ > + if (DECL_DECOMPOSITION_P (decl) && DECL_DECOMP_BASE (decl)) > + TREE_USED (DECL_DECOMP_BASE (decl)) = 1; > > if (TREE_CODE (decl) == TEMPLATE_DECL) > return true; > --- gcc/cp/pt.c.jj 2017-05-24 11:59:02.000000000 +0200 > +++ gcc/cp/pt.c 2017-05-25 13:11:03.326234338 +0200 > @@ -15705,6 +15705,7 @@ tsubst_decomp_names (tree decl, tree pat > return error_mark_node; > } > (*cnt)++; > + gcc_assert (DECL_DECOMP_BASE (decl2) == pattern_decl); > gcc_assert (DECL_HAS_VALUE_EXPR_P (decl2)); > tree v = DECL_VALUE_EXPR (decl2); > DECL_HAS_VALUE_EXPR_P (decl2) = 0; > --- gcc/cp/expr.c.jj 2017-05-24 11:59:02.000000000 +0200 > +++ gcc/cp/expr.c 2017-05-25 13:07:37.304821510 +0200 > @@ -133,8 +133,8 @@ mark_exp_read (tree exp) > switch (TREE_CODE (exp)) > { > case VAR_DECL: > - if (DECL_VALUE_EXPR (exp)) > - mark_exp_read (DECL_VALUE_EXPR (exp)); > + if (DECL_DECOMPOSITION_P (exp)) > + mark_exp_read (DECL_DECOMP_BASE (exp)); > gcc_fallthrough (); > case PARM_DECL: > DECL_READ_P (exp) = 1; > --- gcc/testsuite/g++.dg/cpp1z/decomp29.C.jj 2017-05-24 11:59:01.000000000 +0200 > +++ gcc/testsuite/g++.dg/cpp1z/decomp29.C 2017-05-25 14:47:59.000000000 +0200 > @@ -5,6 +5,7 @@ > struct A { int i,j,k; }; > > A f(); > +int p[3]; > > int z; > > @@ -14,13 +15,42 @@ int main() > auto [i,j,k] = f(); // { dg-warning "unused" } > } > { > + [[maybe_unused]] auto [i,j,k] = f(); > + } > + { > auto [i,j,k] = f(); > z = i; > } > { > + auto [i,j,k] = f(); // { dg-warning "unused" } > + i = 5; > + } > + { > auto [i,j] = std::tuple{1,2}; // { dg-warning "unused" } > } > - // No parallel second test, because in this case i and j are variables rather > - // than mere bindings, so there isn't a link between them and using i will > - // not prevent a warning about unused j. > + { > + [[maybe_unused]] auto [i,j] = std::tuple{1,2}; > + } > + { > + auto [i,j] = std::tuple{1,2}; > + z = i; > + } > + { > + auto [i,j] = std::tuple{1,2}; > + i = 5; > + } > + { > + auto [i,j,k] = p; // { dg-warning "unused" } > + } > + { > + [[maybe_unused]] auto [i,j,k] = p; > + } > + { > + auto [i,j,k] = p; > + z = i; > + } > + { > + auto [i,j,k] = p; // { dg-warning "unused" } > + i = 5; > + } > } > > > Jakub