Re: [C++ PATCH] Fix -Wunused with C++17 structured bindings

[Jason Merrill <jason@redhat.com>]

OK, thanks.

On Thu, May 25, 2017 at 10:22 AM, Jakub Jelinek <jakub@redhat.com> 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 <jakub@redhat.com> 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  <jakub@redhat.com>
>
>         * 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__);      \
>    &lt->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__);      \
> +  &lt->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