diff mbox

Fwd: [PATCH] Attribute for unused warning for variables of non-trivial types

Message ID 201306301321.10663.l.lunak@suse.cz
State New
Headers show

Commit Message

Lubos Lunak June 30, 2013, 11:21 a.m. UTC 
Sorry, this has disappeared off my radar for a while.

On Wednesday 21 of November 2012, Jason Merrill wrote:
> On 11/20/2012 10:39 AM, Lubos Lunak wrote:
> >    if (TREE_CODE (exp) == VAR_DECL || TREE_CODE (exp) == PARM_DECL)
> > -    /* Just know that we've seen something for this node.  */
> > -    TREE_USED (exp) = 1;
> > +    {
> > +      /* Just know that we've seen something for this node.
> > +         Merely creating a warn_unused aggregate doesn't make it used
> > though. */ +      if( !lookup_attribute ("warn_unused", TYPE_ATTRIBUTES
> > (type))) +        TREE_USED (exp) = 1;
> > +    }
>
> I don't think we need this either; without this hunk, we should warn
> with -Wunused-but-set-variable, which seems like the behavior we want,
> since a variable with a non-trivial default constructor is set.

 I have fixed the other two pointed out places (updated version attached), but 
if I do also this, then the warning doesn't work. This is because TREE_USED 
is used for detection, and it triggers the first warning in the relevant part 
in gcc/cp/decl.c . The but-set warning would require using DECL_READ_P being 
unset instead, but that one is always set by that point (it's set at least 
twice during building the ctor call).

 Are you sure this should be covered by -Wunused-but-set-variable rather than 
plain -Wunused-variable? While strictly technically speaking the variable is 
set by the ctor, it's conceptually confusing (is "string s;" really set from 
the developer's point of view?), and it's inconsistent with basic types:

$ echo "void f() { int a = 1; } " | g++ -x c++ -fsyntax-only -Wall -
<stdin>: In function ‘void f()’:
<stdin>:1:16: warning: unused variable ‘a’ [-Wunused-variable]

 So while I could try to patch all the places that mark the variable 
DECL_READ_P during ctor/dtor calls, the only difference I see is that it will 
make the handling more complicated, so I myself do not find that worth it.

Comments

Jason Merrill July 8, 2013, 5:32 p.m. UTC | #1 
On 06/30/2013 04:21 AM, Lubos Lunak wrote:
> Are you sure this should be covered by -Wunused-but-set-variable rather than
> plain -Wunused-variable? While strictly technically speaking the variable is
> set by the ctor, it's conceptually confusing (is "string s;" really set from
> the developer's point of view?), and it's inconsistent with basic types:
>
> $ echo "void f() { int a = 1; } " | g++ -x c++ -fsyntax-only -Wall -
> <stdin>: In function ‘void f()’:
> <stdin>:1:16: warning: unused variable ‘a’ [-Wunused-variable]

Ah, I didn't realize that we warn about unused variables with 
initializers. OK, then.

> +  { "warn_unused",            0, 0, false, false, false,
> +                             handle_warn_unused_attribute, false },

Was it a deliberate decision to put this in the c-common attributes 
rather than the C++-specific ones?  I'm not saying it's wrong, just 
interested in your thinking.

> +               || lookup_attribute ("warn_unused", TYPE_ATTRIBUTES (TREE_TYPE (decl)))))

Line longer than 80 characters.

> +      if( !lookup_attribute ("warn_unused", TYPE_ATTRIBUTES (type)))

Space before paren, not after.

And your patch is missing a ChangeLog entry.

It looks like you don't have a copyright assignment on file with the 
FSF; this patch is small enough not to need one, but we should take care 
of that so that future patches don't get blocked.

Jason
diff mbox

Patch

From 704a0ae2906e090ad08834c78d096af0eff9d1f1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lubo=C5=A1=20Lu=C5=88=C3=A1k?= <l.lunak@suse.cz>
Date: Sun, 30 Jun 2013 13:18:28 +0200
Subject: [PATCH] implement warn_unused attribute (gcc#55203)

---
 gcc/c-family/c-common.c                 | 24 ++++++++++++++++++++++++
 gcc/cp/decl.c                           |  3 ++-
 gcc/cp/init.c                           |  8 ++++++--
 gcc/doc/extend.texi                     | 13 +++++++++++++
 gcc/testsuite/g++.dg/warn/warn_unused.C | 22 ++++++++++++++++++++++
 5 files changed, 67 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/warn_unused.C

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 8b780c2..e4c7f1b 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -368,6 +368,7 @@  static tree handle_optimize_attribute (tree *, tree, tree, int, bool *);
 static tree ignore_attribute (tree *, tree, tree, int, bool *);
 static tree handle_no_split_stack_attribute (tree *, tree, tree, int, bool *);
 static tree handle_fnspec_attribute (tree *, tree, tree, int, bool *);
+static tree handle_warn_unused_attribute (tree *, tree, tree, int, bool *);
 
 static void check_function_nonnull (tree, int, tree *);
 static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT);
@@ -738,6 +739,8 @@  const struct attribute_spec c_common_attribute_table[] =
      The name contains space to prevent its usage in source code.  */
   { "fn spec",	 	      1, 1, false, true, true,
 			      handle_fnspec_attribute, false },
+  { "warn_unused",            0, 0, false, false, false,
+			      handle_warn_unused_attribute, false },
   { NULL,                     0, 0, false, false, false, NULL, false }
 };
 
@@ -7942,6 +7945,27 @@  handle_fnspec_attribute (tree *node ATTRIBUTE_UNUSED, tree ARG_UNUSED (name),
   return NULL_TREE;
 }
 
+/* Handle a "warn_unused" attribute; arguments as in
+   struct attribute_spec.handler.  */
+
+static tree
+handle_warn_unused_attribute (tree *node, tree name,
+			      tree args ATTRIBUTE_UNUSED,
+			      int flags ATTRIBUTE_UNUSED, bool *no_add_attrs)
+{
+  if (TYPE_P (*node))
+    /* Do nothing else, just set the attribute.  We'll get at
+       it later with lookup_attribute.  */
+    ;
+  else
+    {
+      warning (OPT_Wattributes, "%qE attribute ignored", name);
+      *no_add_attrs = true;
+    }
+
+  return NULL_TREE;
+}
+
 /* Handle a "returns_twice" attribute; arguments as in
    struct attribute_spec.handler.  */
 
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index f562546..ae8ef27 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -630,7 +630,8 @@  poplevel (int keep, int reverse, int functionbody)
 	    && DECL_NAME (decl) && ! DECL_ARTIFICIAL (decl)
 	    && type != error_mark_node
 	    && (!CLASS_TYPE_P (type)
-		|| !TYPE_HAS_NONTRIVIAL_DESTRUCTOR (type)))
+		|| !TYPE_HAS_NONTRIVIAL_DESTRUCTOR (type)
+		|| lookup_attribute ("warn_unused", TYPE_ATTRIBUTES (TREE_TYPE (decl)))))
 	  {
 	    if (! TREE_USED (decl))
 	      warning (OPT_Wunused_variable, "unused variable %q+D", decl);
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 4edd150..c6e6f77 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -1505,8 +1505,12 @@  build_aggr_init (tree exp, tree init, int flags, tsubst_flags_t complain)
     }
 
   if (VAR_P (exp) || TREE_CODE (exp) == PARM_DECL)
-    /* Just know that we've seen something for this node.  */
-    TREE_USED (exp) = 1;
+    {
+      /* Just know that we've seen something for this node.
+         Merely creating a warn_unused aggregate doesn't make it used though. */
+      if( !lookup_attribute ("warn_unused", TYPE_ATTRIBUTES (type)))
+        TREE_USED (exp) = 1;
+    }
 
   is_global = begin_init_stmts (&stmt_expr, &compound_stmt);
   destroy_temps = stmts_are_full_exprs_p ();
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index e50f2a4..5874d27 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -16262,6 +16262,19 @@  only be applied to classes declared within an @code{extern "Java"} block.
 Calls to methods declared in this interface are dispatched using GCJ's
 interface table mechanism, instead of regular virtual table dispatch.
 
+@item warn_unused
+@cindex @code{warn_unused} attribute
+
+For C++ types with non-trivial constructors and/or destructors it may be
+difficult or impossible for the compiler to find out whether a variable
+of this type is truly unused if it is not referenced. This type attribute
+informs the compiler that variables of this type should be warned about
+if they appear to be unused, just like variables of basic types would be
+warned about.
+
+Types which would benefit from this type attribute are for example various
+container classes such as std::list or std::string.
+
 @end table
 
 See also @ref{Namespace Association}.
diff --git a/gcc/testsuite/g++.dg/warn/warn_unused.C b/gcc/testsuite/g++.dg/warn/warn_unused.C
new file mode 100644
index 0000000..af687fc
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/warn_unused.C
@@ -0,0 +1,22 @@ 
+// { dg-do compile }
+// { dg-options -Wunused }
+
+struct __attribute__((warn_unused)) Test
+{
+    Test();
+    ~Test();
+    void use();
+};
+
+struct TestNormal
+{
+    TestNormal();
+};
+
+int main()
+{
+   Test unused;         // { dg-warning "unused variable" }
+   Test used;           // { dg-bogus "unused variable" }
+   TestNormal normal;   // { dg-bogus "unused variable" }
+   used.use();
+}
-- 
1.8.1.4