Bug 33255

Summary: A warning for "unused" typedefs?
Product: gcc Reporter: Paolo Carlini <paolo.carlini>
Component: c++Assignee: Dodji Seketeli <dodji>
Status: RESOLVED FIXED    
Severity: enhancement CC: fang, gdr
Priority: P3    
Version: unknown   
Target Milestone: 4.8.0   
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2011-07-22 16:48:39
Bug Depends on: 7263    
Bug Blocks:    
Attachments: Work in progress patch
Woops, this one is the right one.
Enhanced patch with proper ChangeLog
C FE support added

Description Paolo Carlini 2007-08-30 23:21:25 UTC
Just a wild idea, motivated by libstdc++/33084: in that case we had a function with this body:

      typedef _BinClos<_Name, _Constant, _ValArray, _Tp, _Tp> _Closure;
      typedef typename __fun<_Name, _Tp>::result_type _Rt;
      return _Expr<_Closure, _Tp>(_Closure(__t, __v));

where the fact that _Rt was completely pointless clearly hinted to a bug, a typo _Tp -> _Rt in the last line. Would it make sense to add an option to the front-end to warn in such cases?
Comment 1 Andrew Pinski 2007-08-30 23:30:33 UTC
I think it is wrong to warn for unused typedefs, they are all over headers.
Comment 2 Paolo Carlini 2007-08-30 23:33:02 UTC
Careful, only *in function bodies*.
Comment 3 gdr@cs.tamu.edu 2007-08-30 23:43:31 UTC
Subject: Re:  A warning for "unused" typedefs?

On Thu, 30 Aug 2007, pinskia at gcc dot gnu dot org wrote:

| I think it is wrong to warn for unused typedefs, they are all over headers.

In general, I tend to agree with Andrew Pinski on this.  
Maybe the original idea could be refined to *local* typedef
declarations.

-- Gaby
Comment 4 Paolo Carlini 2007-08-30 23:46:07 UTC
(In reply to comment #3)
> Maybe the original idea could be refined to *local* typedef
> declarations.

Of course.
Comment 5 gdr@cs.tamu.edu 2007-08-30 23:51:25 UTC
Subject: Re:  A warning for "unused" typedefs?

On Thu, 30 Aug 2007, pcarlini at suse dot de wrote:

| 
| 
| ------- Comment #4 from pcarlini at suse dot de  2007-08-30 23:46 -------
| (In reply to comment #3)
| > Maybe the original idea could be refined to *local* typedef
| > declarations.
| 
| Of course.

The tricky part is how to determine that a typedef is used.  

-- Gaby
Comment 6 Paolo Carlini 2007-08-30 23:59:16 UTC
Well, assuming there are no "no-go" theorems about that problem ;) I would be certainly interested in studying the problem in better detail...
Comment 7 gdr@cs.tamu.edu 2007-08-31 00:05:55 UTC
Subject: Re:  A warning for "unused" typedefs?

On Thu, 30 Aug 2007, pcarlini at suse dot de wrote:

| Well, assuming there are no "no-go" theorems about that problem ;) I would be
| certainly interested in studying the problem in better detail...

I did not mean to imply that the problem is unsolvable or NP-complete
or something like that.  I just pointed out that usually we rely on

  (1) data flow insfrastructure,
  (2) uniqueness of entities refered to by variable and functions

to warn about unused declarations.

Typedefs on the other hand can be "folded" in very early on.  So, one
needs to track that folding...

-- Gaby
Comment 8 David Fang 2007-08-31 00:33:26 UTC
Aren't unused typedefs sometimes useful for static assertions and concept checking, using templates?  I suppose if one really wanted to keep around an unused typedef, that __attribute__((unused)) might be somehow applicable.  I'm looking forward to c++0x concepts, which will allay some of the need for 'creative' uses of templates.  
Comment 9 gdr@cs.tamu.edu 2007-08-31 01:03:48 UTC
Subject: Re:  A warning for "unused" typedefs?

On Thu, 31 Aug 2007, fang at csl dot cornell dot edu wrote:

| Aren't unused typedefs sometimes useful for static assertions and concept
| checking, using templates?

Maybe.  Do you have examples that involved *local* typedefs?

-- Gaby
Comment 10 Paolo Carlini 2007-08-31 08:41:19 UTC
(In reply to comment #8)
> Aren't unused typedefs sometimes useful for static assertions and concept
> checking, using templates?

I understand the general spirit of your concerns. However I'm under the impression that such tricks are becoming *less* common now that we have a real static_assert in the core language and likewise real concepts.
Comment 11 Paolo Carlini 2007-08-31 08:44:10 UTC
(In reply to comment #7)
> I did not mean to imply that the problem is unsolvable or NP-complete
> or something like that.  I just pointed out that usually we rely on
> 
>   (1) data flow insfrastructure,
>   (2) uniqueness of entities refered to by variable and functions
> 
> to warn about unused declarations.
> 
> Typedefs on the other hand can be "folded" in very early on.  So, one
> needs to track that folding...

I see Gaby, thanks a lot for those details and clarifications.
Comment 12 Paolo Carlini 2008-06-09 13:36:43 UTC
Hi Gaby, just a pointer, this is the enhancement PR I was talking about...
Comment 13 gdr@cs.tamu.edu 2008-06-09 14:06:44 UTC
Subject: Re:  A warning for "unused" typedefs?

"paolo dot carlini at oracle dot com" <gcc-bugzilla@gcc.gnu.org> writes:

| Hi Gaby, just a pointer, this is the enhancement PR I was talking about...

Many thanks, Paolo.

-- Gaby
Comment 14 Paolo Carlini 2011-07-22 17:00:57 UTC
Somebody should add a "Thumb Up" button to Bugzilla.
Comment 15 Dodji Seketeli 2011-07-24 21:57:47 UTC
Created attachment 24820 [details]
Work in progress patch

This works only on the C++ FE (not on the C FE yet) for now and has been lightly tested so far.

I ran it on libstdc++ and so far it fell short only on the use of PB_DS_STATIC_ASSERT (and friends) which defines a typedef.  When this macro is expanded in a function that doesn't use the typedef it defines, the patch warns.  Normally, it shouldn't warn b/c the macro is defined in a system header.  But with the current (libcpp) infrastructure, we cannot tell the difference between a typedef defined in a system header macro that is expanded in a local function, and a typedef that is directly defined in the function.  Normally with the work being done on PR proprocessor/7263 this patch should not warn in those case.  At least if my understanding of the issue is correct.

The warning option is -Wunused-local-typedefs and is activated by -Wall -Wextra, or -Wunused -Wextra.

I am putting it here for now in case someone has interesting test cases to add.
Comment 16 Dodji Seketeli 2011-07-24 22:09:27 UTC
Created attachment 24821 [details]
Woops, this one is the right one.

Sorry the previous patch was the wrong one.
Comment 17 Paolo Carlini 2011-07-24 22:32:22 UTC
Beautiful. And of course I don't think PB_DS_STATIC_ASSERT gonna be a serious problem.
Comment 18 Paolo Carlini 2011-07-24 22:37:39 UTC
By the way, an obvious positive additional testcase, involving templates, would be one inspired by libstdc++/33084 that is involving <valarray> *before* the fix for that bug. Shouldn't be too hard to figure out...
Comment 19 Paolo Carlini 2011-07-24 22:49:32 UTC
Just as a note, wanted also to add that if I understand correctly the (temporary?!?) small issue with system headers, it would also affect people building their code with -D_GLIBCXX_CONCEPT_CHECKS (by itself largely deprecated and not being updated for c++0x). Again, I don't see this as a serious issue if the new warning remains, for now at least, outside -Wall.
Comment 20 Paolo Carlini 2011-07-24 22:54:05 UTC
... or maybe not, because actually _GLIBCXX_CONCEPT_CHECKS defines used typedefs, only, pointless besides type checking.

We should also check -D_GLIBCXX_DEBUG. This one is serious.
Comment 21 dodji@seketeli.org 2011-07-25 11:17:11 UTC
> --- Comment #18 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-07-24 22:37:39 UTC ---
> By the way, an obvious positive additional testcase, involving templates, would
> be one inspired by libstdc++/33084 that is involving <valarray> *before* the
> fix for that bug. Shouldn't be too hard to figure out...

Thanks for the pointer.  I am adding this (hopefully equivalent) test
case to the patch:

+
+template<class T, class U>
+struct S10
+{
+};
+
+template<class T>
+void
+test10(void)
+{
+  typedef typename ST<T>::T bar; // { dg-warning "locally defined but not used" }
+  typedef typename ST<T>::T foo; // We shouldn't warn for this one, as
+				 // it's used below.
+  S10<int, foo> v;
+}
Comment 22 Paolo Carlini 2011-07-25 11:43:47 UTC
Excellent.
Comment 23 dodji@seketeli.org 2011-07-25 14:29:15 UTC
> --- Comment #19 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-07-24 22:49:32 UTC ---
> Just as a note, wanted also to add that if I understand correctly the
> (temporary?!?) small issue with system headers, it would also affect people
> building their code with -D_GLIBCXX_CONCEPT_CHECKS (by itself largely
> deprecated and not being updated for c++0x). Again, I don't see this as a
> serious issue if the new warning remains, for now at least, outside
> -Wall.

Building libstdc++ itself without _GLIBCXX_CONCEPT_CHECKS
raises this warning for e.g:

      typedef typename iterator_traits<_II1>::value_type _ValueType1;
      typedef typename iterator_traits<_II2>::value_type _ValueType2;

 [..]
      __glibcxx_function_requires(_LessThanOpConcept<_ValueType1, _ValueType2>)
      __glibcxx_function_requires(_LessThanOpConcept<_ValueType2, _ValueType1>)

(in libstdc++-v3/include/bits/stl_algobase.h)

As the macro __glibcxx_function_requires is defined to nothing (without
-D_GLIBCXX_CONCEPT_CHECKS) _ValueType1 and _ValueType2 appear unused
here.

So yes, -Wunused-local-typedefs definitely needs to be put out of --Wall
-Extra (and -Wunused).

Thanks.
Comment 24 Dodji Seketeli 2011-07-25 17:31:46 UTC
Created attachment 24833 [details]
Enhanced patch with proper ChangeLog

This version removes the -Wunused-local-typedefs option from -Wunused and -Wall -extra, adds more test cases, fixes some unused local typedefs spots in  libstdc++ and adds a ChangeLog.

Now I guess I need to look at adding support for this option to the C FE...
Comment 25 Dodji Seketeli 2011-07-27 18:09:16 UTC
Created attachment 24845 [details]
C FE support added

This patch adds -Wunused-local-typedefs support to the C FE as well as some due clean up.  I am currently bootstrapping it.
Comment 26 Dodji Seketeli 2011-09-08 13:54:33 UTC
Author: dodji
Date: Thu Sep  8 13:54:24 2011
New Revision: 178692

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=178692
Log:
PR c++/33255 - Support -Wunused-local-typedefs warning

gcc/

	* c-decl.c (lookup_name): Use the new
	maybe_record_typedef_use.
	(pushdecl): Use the new
	record_locally_defined_typedef.
	(store_parm_decls): Allocate cfun->language.
	(finish_function): Use the new maybe_warn_unused_local_typedefs,
	and free cfun->language.
	(c_push_function_context): Allocate cfun->language here only if
	needed.
	(c_pop_function_context): Likewise, mark cfun->language
	for collection only when it should be done.
	* c-common.c (handle_used_attribute): Don't ignore TYPE_DECL
	nodes.
	* c-typeck.c (c_expr_sizeof_type, c_cast_expr): Use the new
	maybe_record_local_typedef_use.

gcc/c-family

	* c-common.h (struct c_language_function::local_typedefs): New
	field.
	(record_locally_defined_typedef, maybe_record_typedef_use)
	(maybe_warn_unused_local_typedefs): Declare new functions.
	* c-common.c (record_locally_defined_typedef)
	(maybe_record_typedef_use)
	(maybe_warn_unused_local_typedefs): Define new functions.
	* c.opt: Declare new -Wunused-local-typedefs flag.

gcc/cp

	* name-lookup.c (pushdecl_maybe_friend_1): Use the new
	record_locally_defined_typedef.
	* decl.c (finish_function): Use the new
	maybe_warn_unused_local_typedefs.
	(grokfield): Use the new record_locally_defined_typedef.
	* parser.c (lookup_name): Use the new maybe_record_typedef_use.

gcc/doc/

	* invoke.texi: Update documentation for -Wunused-local-typedefs.

gcc/testsuite/

	* g++.dg/warn/Wunused-local-typedefs.C: New test file.
	* c-c++-common/Wunused-local-typedefs.c: Likewise.

libstdc++-v3/

	* include/ext/bitmap_allocator.h
	(__detail::__mini_vector::__lower_bound): Remove unused typedef.
	* src/istream.cc (std::operator>>(basic_istream<char>& __in,
	basic_string<char>& __str)): Likewise.
	(std::getline): Likewise.
	* src/valarray.cc (__valarray_product): Likewise.

Added:
    trunk/gcc/testsuite/c-c++-common/Wunused-local-typedefs.c
    trunk/gcc/testsuite/g++.dg/warn/Wunused-local-typedefs.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-decl.c
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-common.c
    trunk/gcc/c-family/c-common.h
    trunk/gcc/c-family/c.opt
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/decl.c
    trunk/gcc/cp/decl2.c
    trunk/gcc/cp/name-lookup.c
    trunk/gcc/cp/parser.c
    trunk/gcc/doc/invoke.texi
    trunk/gcc/testsuite/ChangeLog
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/ext/bitmap_allocator.h
    trunk/libstdc++-v3/src/istream.cc
    trunk/libstdc++-v3/src/valarray.cc
Comment 27 dodji@seketeli.org 2011-09-08 20:55:19 UTC
As a follow-up, I have posted a patchlet[1] to enable this warning whenever
-Wall or -Wunused is used.  This patchlet would be applied when the fix
for PR preprocessor/7263 goes in.

[1]: http://gcc.gnu.org/ml/gcc-patches/2011-09/msg00593.html
Comment 28 Jason Merrill 2011-11-07 17:51:28 UTC
Author: jason
Date: Mon Nov  7 17:51:24 2011
New Revision: 181100

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=181100
Log:
	PR c++/33255
	* decl.c (save_function_data): Clear local_typedefs.

Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/decl.c
Comment 29 Paolo Carlini 2012-08-22 21:00:06 UTC
I suppose we can close this, right? In case there are issues with the implementation, belong to new, separate PRs.