public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Jason Merrill <jason@redhat.com>
Cc: gcc-patches List <gcc-patches@gcc.gnu.org>
Subject: [C++ PATCH] Fix -Wunused with C++17 structured bindings
Date: Thu, 25 May 2017 14:23:00 -0000	[thread overview]
Message-ID: <20170525142232.GF8499@tucnak> (raw)
In-Reply-To: <CADzB+2=Lshw_WbyqYxYDSaVrGj4CSmen5C4y4FqASR6mUHs-eg@mail.gmail.com>

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

  reply	other threads:[~2017-05-25 14:22 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-24  2:23 C++ PATCH to " Jason Merrill
2017-05-24  6:52 ` Jakub Jelinek
2017-05-24 16:19   ` Jason Merrill
2017-05-25 14:23     ` Jakub Jelinek [this message]
2017-05-25 19:08       ` [C++ PATCH] Fix " Jason Merrill

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170525142232.GF8499@tucnak \
    --to=jakub@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jason@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).