diff mbox series

[v7] c++: Fix overeager Woverloaded-virtual with conversion operators [PR109918]

Message ID 010201927b3ed2b9-adcff8a2-f921-4a01-9a48-cf095b74d5d9-000000@eu-west-1.amazonses.com
State New
Headers show
Series [v7] c++: Fix overeager Woverloaded-virtual with conversion operators [PR109918] | expand

Commit Message

Simon Martin Oct. 11, 2024, 11:02 a.m. UTC 
Hi Jason,

On 11 Oct 2024, at 0:35, Jason Merrill wrote:

> On 10/7/24 3:35 PM, Simon Martin wrote:
>> On 7 Oct 2024, at 18:58, Jason Merrill wrote:
>>> On 10/7/24 11:27 AM, Simon Martin wrote:
>
>>>>   	/* Now give a warning for all base functions without overriders,
>>>>   	   as they are hidden.  */
>>>>   	for (tree base_fndecl : base_fndecls)
>>>> +	  {
>>>> +	    if (!base_fndecl || overriden_base_fndecls.contains
>>>> (base_fndecl))
>>>> +	      continue;
>>>> +	    tree *hider = hidden_base_fndecls.get (base_fndecl);
>>>> +	    if (hider)
>>>
>>> How about looping over hidden_base_fndecls instead of base_fndecls?
>
>> Unfortunately it does not work because a given base method can be 
>> hidden
>> by one overload and overriden by another, in which case we don’t 
>> want
>> to warn (see for example AA:foo(int) in Woverloaded-virt7.C). So we 
>> need
>> to take both collections into account.
>
> Yes, you'd still need to check overridden_base_fndecls.contains, but 
> that doesn't seem any different iterating over hidden_base_fndecls 
> instead of base_fndecls.
Sure, and I guess iterating over hidden_base_fndecls is more coherent 

with what the warning is about. Changed in the attached updated patch, 
successfully tested on x86_64-pc-linux-gnu. OK?

Thanks, Simon
From 2fa316a23484828a901144f1e09e68d5a3a6df31 Mon Sep 17 00:00:00 2001
From: Simon Martin <simon@nasilyan.com>
Date: Fri, 11 Oct 2024 10:16:26 +0200
Subject: [PATCH] c++: Fix overeager Woverloaded-virtual with conversion operators [PR109918]

We currently emit an incorrect -Woverloaded-virtual warning upon the
following test case

=== cut here ===
struct A {
  virtual operator int() { return 42; }
  virtual operator char() = 0;
};
struct B : public A {
  operator char() { return 'A'; }
};
=== cut here ===

The problem is that when iterating over ovl_range (fns), warn_hidden
gets confused by the conversion operator marker, concludes that
seen_non_override is true and therefore emits a warning for all
conversion operators in A that do not convert to char, even if
-Woverloaded-virtual is 1 (e.g. with -Wall, the case reported).

A second set of problems is highlighted when -Woverloaded-virtual is 2.

First, with the same test case, since base_fndecls contains all
conversion operators in A (except the one to char, that's been removed
when iterating over ovl_range (fns)), we emit a spurious warning for
the conversion operator to int, even though it's unrelated.

Second, in case there are several conversion operators with different
cv-qualifiers to the same type in A, we rightfully emit a warning,
however the note uses the location of the conversion operator marker
instead of the right one; location_of should go over conv_op_marker.

This patch fixes all these by explicitly keeping track of (1) base
methods that are overriden, as well as (2) base methods that are hidden
but not overriden (and by what), and warning about methods that are in
(2) but not (1). It also ignores non virtual base methods, per
"definition" of -Woverloaded-virtual.

Successfully tested on x86_64-pc-linux-gnu.

	PR c++/109918

gcc/cp/ChangeLog:

	* class.cc (warn_hidden): Keep track of overloaded and of hidden
	base methods. Mention the actual hiding function in the warning,
	not the first overload.
	* error.cc (location_of): Skip over conv_op_marker.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Woverloaded-virt1.C: Check that no warning is
	emitted for non virtual base methods.
	* g++.dg/warn/Woverloaded-virt5.C: New test.
	* g++.dg/warn/Woverloaded-virt6.C: New test.
	* g++.dg/warn/Woverloaded-virt7.C: New test.
	* g++.dg/warn/Woverloaded-virt8.C: New test.
	* g++.dg/warn/Woverloaded-virt9.C: New test.

---
 gcc/cp/class.cc                               | 85 ++++++++++++-------
 gcc/cp/error.cc                               |  3 +-
 gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C |  2 +
 gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C | 12 +++
 gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C | 12 +++
 gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C | 25 ++++++
 gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C | 15 ++++
 gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C | 14 +++
 8 files changed, 135 insertions(+), 33 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C
 create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C
 create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C
 create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C
 create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C

Comments

Jason Merrill Oct. 12, 2024, 2:51 a.m. UTC | #1 
On 10/11/24 7:02 AM, Simon Martin wrote:
> Hi Jason,
> 
> On 11 Oct 2024, at 0:35, Jason Merrill wrote:
> 
>> On 10/7/24 3:35 PM, Simon Martin wrote:
>>> On 7 Oct 2024, at 18:58, Jason Merrill wrote:
>>>> On 10/7/24 11:27 AM, Simon Martin wrote:
>>
>>>>>    	/* Now give a warning for all base functions without overriders,
>>>>>    	   as they are hidden.  */
>>>>>    	for (tree base_fndecl : base_fndecls)
>>>>> +	  {
>>>>> +	    if (!base_fndecl || overriden_base_fndecls.contains
>>>>> (base_fndecl))
>>>>> +	      continue;
>>>>> +	    tree *hider = hidden_base_fndecls.get (base_fndecl);
>>>>> +	    if (hider)
>>>>
>>>> How about looping over hidden_base_fndecls instead of base_fndecls?
>>
>>> Unfortunately it does not work because a given base method can be
>>> hidden
>>> by one overload and overriden by another, in which case we don’t
>>> want
>>> to warn (see for example AA:foo(int) in Woverloaded-virt7.C). So we
>>> need
>>> to take both collections into account.
>>
>> Yes, you'd still need to check overridden_base_fndecls.contains, but
>> that doesn't seem any different iterating over hidden_base_fndecls
>> instead of base_fndecls.
> Sure, and I guess iterating over hidden_base_fndecls is more coherent
> 
> with what the warning is about. Changed in the attached updated patch,
> successfully tested on x86_64-pc-linux-gnu. OK?

OK, thanks.

Jason
diff mbox series

Patch

diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index 646072d4f20..f754886a60a 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -3243,10 +3243,15 @@  warn_hidden (tree t)
 	  continue;
 
 	tree name = OVL_NAME (fns);
+	size_t num_fns = 0; /* The number of fndecls in fns.  */
 	auto_vec<tree, 20> base_fndecls;
 	tree base_binfo;
 	tree binfo;
 	unsigned j;
+	hash_set<tree> overriden_base_fndecls;
+	/* base_fndecls that are hidden but not overriden. The "value"
+	   contains the fndecl that hides the "key".  */
+	hash_map<tree, tree> hidden_base_fndecls;
 
 	if (IDENTIFIER_CDTOR_P (name))
 	  continue;
@@ -3264,47 +3269,63 @@  warn_hidden (tree t)
 	if (base_fndecls.is_empty ())
 	  continue;
 
-	/* Remove any overridden functions.  */
-	bool seen_non_override = false;
+	/* Find all the base_fndecls that are overridden, as well as those
+	   that are hidden, in T.  */
 	for (tree fndecl : ovl_range (fns))
 	  {
-	    bool any_override = false;
+	    fndecl = STRIP_TEMPLATE (fndecl);
 	    if (TREE_CODE (fndecl) == FUNCTION_DECL
-		&& DECL_VINDEX (fndecl))
+		&& fndecl != conv_op_marker)
 	      {
-		/* If the method from the base class has the same
-		   signature as the method from the derived class, it
-		   has been overridden.  Note that we can't move on
-		   after finding one match: fndecl might override
-		   multiple base fns.  */
+		num_fns++;
+		tree fndecl_vindex = DECL_VINDEX (fndecl);
+		/* If the method from the base class has the same DECL_VINDEX
+		   as the method from the derived, it has been overridden.
+		   Note that we can't move on after finding one match: fndecl
+		   might override multiple base fns.  */
 		for (size_t k = 0; k < base_fndecls.length (); k++)
-		  if (base_fndecls[k]
-		      && same_signature_p (fndecl, base_fndecls[k]))
-		    {
-		      base_fndecls[k] = NULL_TREE;
-		      any_override = true;
-		    }
+		  {
+		    if (!base_fndecls[k] || !DECL_VINDEX (base_fndecls[k]))
+		      continue;
+		    tree base_fndecl_vindex = DECL_VINDEX (base_fndecls[k]);
+		    if (fndecl_vindex
+			&& !tree_int_cst_compare (fndecl_vindex,
+						  base_fndecl_vindex))
+		      overriden_base_fndecls.add (base_fndecls[k]);
+		    else if (IDENTIFIER_CONV_OP_P (name)
+			     && (DECL_NAME (fndecl)
+				 != DECL_NAME (base_fndecls[k])))
+		      /* If base_fndecl[k] and fndecl are conversion operators
+			 to different types, they're unrelated.  */
+		      ;
+		    else
+		      hidden_base_fndecls.put (base_fndecls[k], fndecl);
+		  }
 	      }
-	    if (!any_override)
-	      seen_non_override = true;
 	  }
 
-	if (!seen_non_override && warn_overloaded_virtual == 1)
-	  /* All the derived fns override base virtuals.  */
-	  return;
+	if (warn_overloaded_virtual == 1
+	    && overriden_base_fndecls.elements () == num_fns)
+	  /* All the fns override a base virtual.  */
+	  continue;
 
-	/* Now give a warning for all base functions without overriders,
-	   as they are hidden.  */
-	for (tree base_fndecl : base_fndecls)
-	  if (base_fndecl)
-	    {
-	      auto_diagnostic_group d;
-	      /* Here we know it is a hider, and no overrider exists.  */
-	      if (warning_at (location_of (base_fndecl),
-			      OPT_Woverloaded_virtual_,
-			      "%qD was hidden", base_fndecl))
-		inform (location_of (fns), "  by %qD", fns);
-	    }
+	/* Now give a warning for all hidden methods. Note that a method that
+	   is both in hidden_base_fndecls and overriden_base_fndecls is not
+	   hidden.  */
+	for (auto hidden_base_fndecl : hidden_base_fndecls)
+	  {
+	    tree hidden_fndecl = hidden_base_fndecl.first;
+	    tree hider = hidden_base_fndecl.second;
+	    if (!hidden_fndecl
+		|| overriden_base_fndecls.contains (hidden_fndecl))
+	      continue;
+	    gcc_assert (hider);
+	    auto_diagnostic_group d;
+	    if (warning_at (location_of (hidden_fndecl),
+			    OPT_Woverloaded_virtual_,
+			    "%qD was hidden", hidden_fndecl))
+	      inform (location_of (hider), "  by %qD", hider);
+	  }
       }
 }
 
diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc
index 65f70c595cf..3d0dad4ee59 100644
--- a/gcc/cp/error.cc
+++ b/gcc/cp/error.cc
@@ -3397,7 +3397,8 @@  location_of (tree t)
 	return input_location;
     }
   else if (TREE_CODE (t) == OVERLOAD)
-    t = OVL_FIRST (t);
+    t = OVL_FIRST (t) != conv_op_marker ? OVL_FIRST (t)
+      : OVL_FIRST (OVL_CHAIN (t));
 
   if (DECL_P (t))
     return DECL_SOURCE_LOCATION (t);
diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C
index 92f8327b9d0..9091bfabc96 100644
--- a/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C
@@ -5,10 +5,12 @@  class Foo
 {
 public:
     virtual void f(int);	// { dg-warning "hidden" }
+    void g(int);		// Not virtual, so no warning
 };
 
 class Bar : public Foo
 {
 public:
     virtual void f(short);	// { dg-message "by" }
+    virtual void g(short);
 };
diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C
new file mode 100644
index 00000000000..01cd562609a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C
@@ -0,0 +1,12 @@ 
+// PR c++/109918 - Exact PR testcase
+// { dg-do compile }
+// { dg-additional-options -Wall }
+
+struct A {
+  virtual operator int() { return 42; }
+  virtual operator char() = 0;
+};
+
+struct B : public A {
+  operator char() { return 'A'; }
+};
diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C
new file mode 100644
index 00000000000..c18049e3a92
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C
@@ -0,0 +1,12 @@ 
+// PR c++/109918 - PR testcase with -Woverloaded-virtual=2
+// { dg-do compile }
+// { dg-additional-options -Woverloaded-virtual=2 }
+
+struct A {
+  virtual operator int() { return 42; }
+  virtual operator char() = 0;
+};
+
+struct B : public A {
+  operator char() { return 'A'; }
+};
diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C
new file mode 100644
index 00000000000..be50c890d20
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C
@@ -0,0 +1,25 @@ 
+// PR c++/109918 - Test different CV-quals
+// { dg-do compile }
+// { dg-additional-options -Woverloaded-virtual }
+
+struct A {
+  virtual operator char() { return 'a'; }
+  virtual operator char() const { return 'b'; } // { dg-warning "was hidden" }
+  virtual operator int() { return 42; }
+};
+
+struct B : public A {
+  operator char() { return 'A'; } // { dg-note "by" }
+  operator int() { return 43; }
+};
+
+struct AA {
+  virtual char func(char) { return 'a'; }
+  virtual char func(char) const { return 'b'; } // { dg-warning "was hidden" }
+  virtual int func(int) { return 42; }
+};
+
+struct BB : public AA {
+  char func(char) { return 'A'; } // { dg-note "by" }
+  int func(int) { return 43; }
+};
diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C
new file mode 100644
index 00000000000..51af2dae77c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C
@@ -0,0 +1,15 @@ 
+// Identified when investigating PR c++/109918: no warning was emitted due to
+// an incorrect early return in warn_hidden.
+// { dg-additional-options -Wall }
+
+struct Foo
+{
+  virtual void f(int); // { dg-warning "hidden" }
+  virtual void g() {}
+};
+
+struct Bar : Foo
+{
+  virtual void f(short); // { dg-message "by" }
+  virtual void g() {}
+};
diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C
new file mode 100644
index 00000000000..6d315c63a08
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C
@@ -0,0 +1,14 @@ 
+// PR c++/109918: Non virtual overloads in derived classes that don't override
+// anything shouldn't cause warnings, even at -Woverloaded-virtual=2
+// { dg-additional-options -Woverloaded-virtual=2 }
+
+struct Foo
+{
+  virtual void g() {}
+};
+
+struct Bar : Foo
+{
+  virtual void g() {}
+  void g(int) {}
+};