diff mbox

[C++] -Wunused-but-set-parameter fix followup (PR c++/79782)

Message ID 20170301194019.GF1849@tucnak
State New
Headers show

Commit Message

Jakub Jelinek March 1, 2017, 7:40 p.m. UTC 
Hi!

On Tue, Feb 28, 2017 at 02:47:31PM -1000, Nathan Sidwell wrote:
> On 02/28/2017 02:41 PM, Jason Merrill wrote:
> > On Tue, Feb 28, 2017 at 12:48 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > > The DR1659/DR1611 changes result in construct_virtual_base not being called,
> > > but unfortunately the call generated in there was the spot that caused
> > > mark_exp_read on the arguments passed to the vbase construction (TREE_USED
> > > is set on these earlier already during parsing them).  That results
> > > in false positive -Wunused-but-set-parameter warnings.
> > > 
> > > The following patch tries to avoid the warning in that case by marking
> > > the arguments as read (essentially pretending they were read in the omitted
> > > call).  Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > I believe there's some question still about whether the DR1659/11
> > changes are what we want; I'll defer to Nathan on this patch.
> 
> Jakub's patch is OK.  The defect I've reported is how 1658 interacts with
> virtual destructors. (bug 79393)

Unfortunately my patch apparently broke the case where such ctor in virtual
class has no arguments (void_type_node is used in that case instead of a
TREE_LIST, it is a little bit weird (I'd have expected perhaps
void_list_node instead), but it is what it does).

Plus, as the following testcase shows, mark_exp_read really expects to be
called on quite narrow sets of expressions that are being emitted, while
in arguments because it is not really emitted we can have e.g. nested
CONSTRUCTORs etc. and mark_exp_read doesn't handle those.

So this patch in addition to not walking anything for
arguments == void_type_node
just walks the arguments and calls mark_exp_read on all PARM_DECLs in there
(I think that is all we care about, we can't have there VAR_DECLs or
RESULT_DECLs).

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-03-01  Jakub Jelinek  <jakub@redhat.com>

	PR c++/79782
	* init.c (mark_exp_read_r): New function.
	(emit_mem_initializers): Don't walk arguments if it is void_type_node.
	Use cp_walk_tree with mark_exp_read_r instead of plain mark_exp_read.

	* g++.dg/warn/Wunused-parm-10.C: New test.



	Jakub

Comments

Nathan Sidwell March 2, 2017, 1:10 a.m. UTC | #1 
On 03/01/2017 09:40 AM, Jakub Jelinek wrote:

> Unfortunately my patch apparently broke the case where such ctor in virtual
> class has no arguments (void_type_node is used in that case instead of a
> TREE_LIST, it is a little bit weird (I'd have expected perhaps
> void_list_node instead), but it is what it does).

Seems funky (but not your problem), which of the testcase(s) does it correspond to?

> So this patch in addition to not walking anything for
> arguments == void_type_node
> just walks the arguments and calls mark_exp_read on all PARM_DECLs in there
> (I think that is all we care about, we can't have there VAR_DECLs or
> RESULT_DECLs).

I suppose someone could pass in a global VAR_DECL, but if the ctor's the only 
use of that decl, it's rather stupid.

Do you actually need to iterate over the arg list -- can't you just pass 
ARGUMENTS straight into cp_walk_tree?

Ok with or without that change.

nathan
diff mbox

Patch

--- gcc/cp/init.c.jj	2017-03-01 09:35:19.000000000 +0100
+++ gcc/cp/init.c	2017-03-01 16:59:09.014981469 +0100
@@ -1127,6 +1127,17 @@  sort_mem_initializers (tree t, tree mem_
   return sorted_inits;
 }
 
+/* Callback for cp_walk_tree to mark all PARM_DECLs in a tree as read.  */
+
+static tree
+mark_exp_read_r (tree *tp, int *, void *)
+{
+  tree t = *tp;
+  if (TREE_CODE (t) == PARM_DECL)
+    mark_exp_read (t);
+  return NULL_TREE;
+}
+
 /* Initialize all bases and members of CURRENT_CLASS_TYPE.  MEM_INITS
    is a TREE_LIST giving the explicit mem-initializer-list for the
    constructor.  The TREE_PURPOSE of each entry is a subobject (a
@@ -1217,12 +1228,12 @@  emit_mem_initializers (tree mem_inits)
 	/* C++14 DR1658 Means we do not have to construct vbases of
 	   abstract classes.  */
 	construct_virtual_base (subobject, arguments);
-      else
+      else if (arguments != void_type_node)
 	/* When not constructing vbases of abstract classes, at least mark
 	   the arguments expressions as read to avoid
 	   -Wunused-but-set-parameter false positives.  */
 	for (tree arg = arguments; arg; arg = TREE_CHAIN (arg))
-	  mark_exp_read (TREE_VALUE (arg));
+	  cp_walk_tree (&TREE_VALUE (arg), mark_exp_read_r, NULL, NULL);
 
       if (inherited_base)
 	pop_deferring_access_checks ();
--- gcc/testsuite/g++.dg/warn/Wunused-parm-10.C.jj	2017-03-01 17:02:41.811195793 +0100
+++ gcc/testsuite/g++.dg/warn/Wunused-parm-10.C	2017-03-01 17:01:31.000000000 +0100
@@ -0,0 +1,12 @@ 
+// PR c++/79782
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wunused-but-set-parameter -Wunused-parameter" }
+
+struct E { virtual E *foo () const = 0; };
+struct F : virtual public E { };
+struct G : public virtual F { G (int x) : F () { } };				// { dg-warning "unused parameter" }
+struct H : virtual public E { H (int x, int y); };
+struct I : public virtual H { I (int x, int y) : H (x, y) { } };		// { dg-bogus "set but not used" }
+struct J : public virtual H { J (int x, int y) : H { x, y } { } };		// { dg-bogus "set but not used" }
+struct K : public virtual H { K (int x, int y) : H (x * 0, y + 1) { } };	// { dg-bogus "set but not used" }
+struct L : public virtual H { L (int x, int y) : H { x & 0, y | 1 } { } };	// { dg-bogus "set but not used" }