[Patch c++] (__attribute__((deprecated)), part a) Fix PR17729

[Patch c++] (__attribute__((deprecated)), part a) Fix PR17729

[IainS]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

Split-up of http://gcc.gnu.org/ml/gcc-patches/2010-04/msg01037.html
Part a.

I guess the testcase is somewhat moot because of  http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30612
... but I included for completeness.

regtested on i686-apple-darwin9

OK for trunk? (and since this dates back to 4.3 .. I guess at least  
4.5 ? )
Iain.

gcc/cp/Changelog:

	PR c++/17729
	* typeck.c(build_class_member_access_expr): Don't check for  
deprecation here.
	* call.c(build_call_a): Ditto.
	(build_over_call): Ditto.
	(convert_like_real): Check for deprecation here instead.
	(build_new_method_call): Ditto.

gcc/testsuite/Changelog:

	PR c++/17729
	* g++.dg/warm/deprecated-7.C: New.



[-- Attachment #2: 158459-cp-dup-errs-diff.txt --]
[-- Type: text/plain, Size: 3664 bytes --]

Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 158459)
+++ gcc/cp/typeck.c	(working copy)
@@ -2149,9 +2149,7 @@ build_class_member_access_expr (tree object, tree
     {
       member_scope = DECL_CLASS_CONTEXT (member);
       mark_used (member);
-      if (TREE_DEPRECATED (member))
-	warn_deprecated_use (member, NULL_TREE);
-    }
+   }
   else
     member_scope = BINFO_TYPE (BASELINK_ACCESS_BINFO (member));
   /* If MEMBER is from an anonymous aggregate, MEMBER_SCOPE will
Index: gcc/cp/call.c
===================================================================
--- gcc/cp/call.c	(revision 158459)
+++ gcc/cp/call.c	(working copy)
@@ -344,8 +344,6 @@ build_call_a (tree function, int n, tree *argarray
   if (decl && TREE_THIS_VOLATILE (decl) && cfun && cp_function_chain)
     current_function_returns_abnormally = 1;
 
-  if (decl && TREE_DEPRECATED (decl))
-    warn_deprecated_use (decl, NULL_TREE);
   require_complete_eh_spec_types (fntype, decl);
 
   if (decl && DECL_CONSTRUCTOR_P (decl))
@@ -4904,6 +4902,10 @@ convert_like_real (conversion *convs, tree expr, t
 	for (i = 0; i < cand->num_convs; ++i)
 	  cand->convs[i]->user_conv_p = true;
 
+	/* Warn on deprecation of the candidate conversion func. */
+	if (TREE_DEPRECATED (convfn))
+	  warn_deprecated_use (convfn, NULL_TREE);
+
 	expr = build_over_call (cand, LOOKUP_NORMAL, complain);
 
 	/* If this is a constructor or a function returning an aggr type,
@@ -5864,11 +5866,6 @@ build_over_call (struct z_candidate *cand, int fla
 				ba_any, NULL);
       gcc_assert (binfo && binfo != error_mark_node);
 
-      /* Warn about deprecated virtual functions now, since we're about
-	 to throw away the decl.  */
-      if (TREE_DEPRECATED (fn))
-	warn_deprecated_use (fn, NULL_TREE);
-
       argarray[0] = build_base_path (PLUS_EXPR, argarray[0], binfo, 1);
       if (TREE_SIDE_EFFECTS (argarray[0]))
 	argarray[0] = save_expr (argarray[0]);
@@ -6458,6 +6455,11 @@ build_new_method_call (tree instance, tree fns, VE
 	      /* Now we know what function is being called.  */
 	      if (fn_p)
 		*fn_p = fn;
+
+	      /* Check for deprecation of the function to the called. */
+	      if (TREE_DEPRECATED (fn))
+		warn_deprecated_use (fn, NULL_TREE);
+	      
 	      /* Build the actual CALL_EXPR.  */
 	      call = build_over_call (cand, flags, complain);
 	      /* In an expression of the form `a->f()' where `f' turns
Index: gcc/testsuite/g++.dg/warn/deprecated-7.C
===================================================================
--- gcc/testsuite/g++.dg/warn/deprecated-7.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/deprecated-7.C	(revision 0)
@@ -0,0 +1,34 @@
+// PR c++/17729 Multiple warnings for single errors.
+// { dg-do compile }
+
+void func(void) __attribute__((deprecated));
+
+void f(void) {
+  func(); // { dg-warning "'void func\\(\\)' is deprecated" "func()" }	
+// { dg-warning "'void func\\(\\)' is deprecated" "func()" { ! target *-*-* } 7 }	
+}
+
+struct s1 {
+  int notdep ;
+  int depfield __attribute__((deprecated));
+  union {
+    int notdep;
+    int deprec __attribute__((deprecated));
+  } u1 ;
+  union {
+    int notdep1;
+    int notdep2;
+  } u2 __attribute__((deprecated)) ;
+ 
+} ;
+
+int main(void) { 
+ struct s1 a;
+ int b;
+ b  = a.notdep;
+ b += a.depfield; // { dg-warning "'s1::depfield' is deprecated .declared at" "depfield" }
+ b += a.u1.notdep;
+ b += a.u1.deprec; // { dg-warning "'s1::<anonymous union>::deprec' is deprecated .declared at" "s1::u1.deprec" }
+ b += a.u2.notdep1 ;// { dg-warning "'s1::u2' is deprecated .declared at" "s1::u2" }
+ return 0;
+}

[-- Attachment #3: Type: text/plain, Size: 1 bytes --]

Re: [Patch c++] (__attribute__((deprecated)), part a) Fix PR17729

[Jason Merrill]

On 04/17/2010 07:16 AM, IainS wrote:
> PR c++/17729
> * typeck.c(build_class_member_access_expr): Don't check for deprecation
> here.

I believe this will cause false negatives on template code.

> * call.c(build_call_a): Ditto.
> (build_over_call): Ditto.
> (convert_like_real): Check for deprecation here instead.
> (build_new_method_call): Ditto.

And these cause false negatives on overloaded functions.  We really need 
to handle functions in build_call_a and build_over_call, since that's 
when we know which function we're actually calling in the case of 
function overloading.  In the testcase, if you add a non-deprecated 
func(int), you get no warning about calling the deprecated func(void). 
The testcase also doesn't test member functions at all.

Sorry for the slow review; please feel free to ping me directly on C++ 
patches.

Jason

Re: [Patch c++] (__attribute__((deprecated)), part a) Fix PR17729

[IainS]

On 8 Jun 2010, at 23:50, Jason Merrill wrote:

> On 04/17/2010 07:16 AM, IainS wrote:
>> PR c++/17729
>> * typeck.c(build_class_member_access_expr): Don't check for  
>> deprecation
>> here.
>
> I believe this will cause false negatives on template code.
>
>> * call.c(build_call_a): Ditto.
>> (build_over_call): Ditto.
>> (convert_like_real): Check for deprecation here instead.
>> (build_new_method_call): Ditto.
>
> And these cause false negatives on overloaded functions.  We really  
> need to handle functions in build_call_a and build_over_call, since  
> that's when we know which function we're actually calling in the  
> case of function overloading.

the question then, is how to prevent the duplicates, since those  
places are called from more than one part of the process (or, at  
least, they were when I last looked).

>  In the testcase, if you add a non-deprecated func(int), you get no  
> warning about calling the deprecated func(void). The testcase also  
> doesn't test member functions at all.

OK.  I wasn't amazingly happy that this had all bases covered.
I expanded the test-cases a little - but they are mostly based on  
what's already in the test-suite.

----

Actually, I intended to get back to this in the next few weeks - since  
__attribute__((unavailable)) is a prerequisite of ObjC V1.

Before I put this to one side, I had improved the diagnosis of  
deprecated function arguments - and also harmonized the behavior of  
function deprecation messages between C and C++.

I am sure all those patches will have severe bit-rot now - but I'll  
try and resurrect them soon.

> Sorry for the slow review; please feel free to ping me directly on C+ 
> + patches.

thanks, I'm going slowly here anyway,
Iain

Re: [Patch c++] (__attribute__((deprecated)), part a) Fix PR17729

[Jason Merrill]

On 06/08/2010 07:13 PM, IainS wrote:
> the question then, is how to prevent the duplicates, since those places
> are called from more than one part of the process (or, at least, they
> were when I last looked).

For non-function members, I think removing the call in 
finish_class_member_access_expr ought to do the trick, letting 
build_class_member_access_expr handle the warning.

For functions, perhaps just warning in mark_used would work.  Then 
removing the calls in build_over_call and build_call_a and conditioning 
the one in finish_id_expression on !is_overloaded_fn.

Jason

Re: [Patch c++] (__attribute__((deprecated)), part a) Fix PR17729

[IainS]

[-- Attachment #1: Type: text/plain, Size: 2184 bytes --]

Hi Jason,

It's taken a while to find some more time for this - (ObJC LTO and  
emutls.... etc. etc.)

On 9 Jun 2010, at 04:19, Jason Merrill wrote:

> On 06/08/2010 07:13 PM, IainS wrote:
>> the question then, is how to prevent the duplicates, since those  
>> places
>> are called from more than one part of the process (or, at least, they
>> were when I last looked).
>
> For non-function members, I think removing the call in  
> finish_class_member_access_expr ought to do the trick, letting  
> build_class_member_access_expr handle the warning.

done

> For functions, perhaps just warning in mark_used would work.  Then  
> removing the calls in build_over_call and build_call_a and  
> conditioning the one in finish_id_expression on !is_overloaded_fn.

unfortunately, not
mark_used is called even more often than build_over_call &  
build_call_a (at one stage I ended up with three warning messages)...

It seems that there is no nice place to put these diagnostics, that is  
only visited once (for the current formulation***);
Hence we end up checking in several places (which, I agree, does not  
seem ideal).

The attached patch has been bootstrapped/checked on i686-apple-darwin9

I've added a test-case for overloaded functions, member funcs are  
covered in deprecated-{4,6} iirc.

thoughts?

Iain

*** It seems to me that the diagnostics might be better done at the  
stage that the parser has decided that (other than deprecation) a  
given construct is OK.
(I guess just before swallowing the ";").   However, I don't know if  
this is reasonable, &| there would be enough information preserved to  
make the diagnostic neat (although it seems that there should).


gcc/cp:

	PR c++/17729
	* typeck.c (finish_class_member_access_expr): Remove duplicate
	deprecation check.
	* call.c (build_call_a): Do not check deprecation here.
	(build_over_call): Ditto.
	(build_new_function_call): Check for deprecated overloaded funcs
	here.
	(convert_like_real): Check for deprecated functions.
	(build_new_method_call): Ditto.

gcc/testsuite:

	PR c++/17729
	* g++.dg/warn/pr17729-1-fields.C: New.
	* g++.dg/warn/pr17729-2-funcs.C: New.
	* g++.dg/warn/deprecated-7.C: New.


[-- Attachment #2: 162302-pr17729.txt --]
[-- Type: text/plain, Size: 5090 bytes --]

Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 162309)
+++ gcc/cp/typeck.c	(working copy)
@@ -2692,9 +2692,6 @@ finish_class_member_access_expr (tree object, tree
 	}
     }
 
-  if (TREE_DEPRECATED (member))
-    warn_deprecated_use (member, NULL_TREE);
-
   if (template_p)
     check_template_keyword (member);
 
Index: gcc/cp/call.c
===================================================================
--- gcc/cp/call.c	(revision 162309)
+++ gcc/cp/call.c	(working copy)
@@ -347,8 +347,6 @@ build_call_a (tree function, int n, tree *argarray
   if (decl && TREE_THIS_VOLATILE (decl) && cfun && cp_function_chain)
     current_function_returns_abnormally = 1;
 
-  if (decl && TREE_DEPRECATED (decl))
-    warn_deprecated_use (decl, NULL_TREE);
   require_complete_eh_spec_types (fntype, decl);
 
   if (decl && DECL_CONSTRUCTOR_P (decl))
@@ -3283,8 +3281,11 @@ build_new_function_call (tree fn, VEC(tree,gc) **a
       result = error_mark_node;
     }
   else
-    result = build_over_call (cand, LOOKUP_NORMAL, complain);
-
+    {
+      if (koenig_p && TREE_DEPRECATED (cand->fn))
+	warn_deprecated_use (cand->fn, NULL_TREE);
+      result = build_over_call (cand, LOOKUP_NORMAL, complain);
+    }
   /* Free all the conversions we allocated.  */
   obstack_free (&conversion_obstack, p);
 
@@ -5018,6 +5019,10 @@ convert_like_real (conversion *convs, tree expr, t
 	for (i = 0; i < cand->num_convs; ++i)
 	  cand->convs[i]->user_conv_p = true;
 
+	/* Warn on deprecation of the candidate conversion func.  */
+	if (TREE_DEPRECATED (convfn))
+	  warn_deprecated_use (convfn, NULL_TREE);
+
 	expr = build_over_call (cand, LOOKUP_NORMAL, complain);
 
 	/* If this is a constructor or a function returning an aggr type,
@@ -6010,11 +6015,6 @@ build_over_call (struct z_candidate *cand, int fla
 				ba_any, NULL);
       gcc_assert (binfo && binfo != error_mark_node);
 
-      /* Warn about deprecated virtual functions now, since we're about
-	 to throw away the decl.  */
-      if (TREE_DEPRECATED (fn))
-	warn_deprecated_use (fn, NULL_TREE);
-
       argarray[0] = build_base_path (PLUS_EXPR, argarray[0], binfo, 1);
       if (TREE_SIDE_EFFECTS (argarray[0]))
 	argarray[0] = save_expr (argarray[0]);
@@ -6575,6 +6575,11 @@ build_new_method_call (tree instance, tree fns, VE
 	      /* Now we know what function is being called.  */
 	      if (fn_p)
 		*fn_p = fn;
+
+	      /* Check for deprecation of the function to be called.  */
+	      if (TREE_DEPRECATED (fn))
+		warn_deprecated_use (fn, NULL_TREE);
+	      
 	      /* Build the actual CALL_EXPR.  */
 	      call = build_over_call (cand, flags, complain);
 	      /* In an expression of the form `a->f()' where `f' turns
Index: gcc/testsuite/g++.dg/warn/pr17729-2-funcs.C
===================================================================
--- gcc/testsuite/g++.dg/warn/pr17729-2-funcs.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/pr17729-2-funcs.C	(revision 0)
@@ -0,0 +1,9 @@
+/* PR 17729 */
+/* { dg-do compile } */
+int fun (void) __attribute__((deprecated("no fun"))) ;
+
+void foo (void)
+{
+  fun (); // { dg-bogus "int fun.* no fun.*int fun" "duplicate func. warning" }
+	  // { dg-warning "'int fun..' is deprecated .declared at \[^\\)\]*.: no fun" "" { target *-*-* } 7 }
+}
Index: gcc/testsuite/g++.dg/warn/deprecated-7.C
===================================================================
--- gcc/testsuite/g++.dg/warn/deprecated-7.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/deprecated-7.C	(revision 0)
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+
+int f1 (int, int);
+int f1 (float, int) __attribute__((deprecated("no flt, int"))) ;
+int f1 (float, float);
+
+void foo (void)
+{
+  int a, b, r;
+  float c,d;
+  
+  r = f1 (a, b);
+  r = f1 (c, a); // { dg-warning "'int f1.float, int.' is deprecated .declared at \[^\\)\]*.: no flt, int" }
+  r = f1 (c, d);
+
+}
\ No newline at end of file
Index: gcc/testsuite/g++.dg/warn/pr17729-1-fields.C
===================================================================
--- gcc/testsuite/g++.dg/warn/pr17729-1-fields.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/pr17729-1-fields.C	(revision 0)
@@ -0,0 +1,20 @@
+/* PR 17729 */
+/* { dg-do compile } */
+typedef int INT1 __attribute__((deprecated("no INT1"))) ;
+
+struct _a {
+  int field1 ;
+  int field2 __attribute__((deprecated("no field2"))) ;
+  INT1 field3; 						/* { dg-warning "'INT1' is deprecated .declared \[^\\)\]*.: no INT1 " } */
+  INT1 field4 __attribute__((deprecated("no field4"))); /* { dg-warning "'INT1' is deprecated .declared .* no INT1 " } */
+} s;
+
+void foo (void)
+{
+  s.field1 = 1;
+  s.field2 = 2; // { dg-bogus "_a::field2.*_a::field2" "duplicate field warning" }
+  		// { dg-warning "'_a::field2' is deprecated .declared at \[^\\)\]*.: no field2" "" { target *-*-* } 15 }
+  s.field3 = 3;
+  s.field4 = 4; // { dg-bogus "_a::field4.*_a::field4" "duplicate field warning" }
+  		// { dg-warning "'_a::field4' is deprecated .declared at .* no field4" "" { target *-*-* } 18 }
+}
\ No newline at end of file

[-- Attachment #3: Type: text/plain, Size: 2 bytes --]

Re: [Patch c++] (__attribute__((deprecated)), part a) Fix PR17729

[Jason Merrill]

On 07/19/2010 01:11 PM, IainS wrote:
> unfortunately, not
> mark_used is called even more often than build_over_call & build_call_a
> (at one stage I ended up with three warning messages)...
>
> It seems that there is no nice place to put these diagnostics, that is
> only visited once (for the current formulation***);
> Hence we end up checking in several places (which, I agree, does not
> seem ideal).

How about just using a hash table, as in maybe_explain_implicit_delete?

Jason

Re: [Patch c++] (__attribute__((deprecated)), part a) Fix PR17729

[IainS]

On 19 Jul 2010, at 18:58, Jason Merrill wrote:

> On 07/19/2010 01:11 PM, IainS wrote:
>> unfortunately, not
>> mark_used is called even more often than build_over_call &  
>> build_call_a
>> (at one stage I ended up with three warning messages)...
>>
>> It seems that there is no nice place to put these diagnostics, that  
>> is
>> only visited once (for the current formulation***);
>> Hence we end up checking in several places (which, I agree, does not
>> seem ideal).
>
> How about just using a hash table, as in  
> maybe_explain_implicit_delete?

I did some more back-tracing yesterday (to find where the duplicate  
calls are coming from);
I'll analyze that a bit more later, and maybe augment it ...

one point that this revealed is the types are not marked as used (at  
least not in this manner).
presumably there is some equivalent action so that we know which ones  
to emit for debug?

cheers,
Iain