[C++ PATCH] vfunc overrider simplification

[C++ PATCH] vfunc overrider simplification

[Nathan Sidwell]

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

In fixing a vfunc override bug on the modules branch, I noticed that 
check_for_override can simply check IDENTIFIER_VIRTUAL_P -- the dtor 
identifier and those for conversion operators will have it set 
correctly.  (there a multiple conversion operator identifiers, but we 
can only be overriding the one with the same return type, so that's fine.)

While there I moved the DECL_OVERRIDE_P check up and avoid needing a 
bool flag.

committing to trunk.

nathan

-- 
Nathan Sidwell

[-- Attachment #2: vfunc.diff --]
[-- Type: text/x-patch, Size: 2865 bytes --]

2019-08-23  Nathan Sidwell  <nathan@acm.org>

	* class.c (check_for_override): Checking IDENTIFIER_VIRTUAL_P is
	sufficient, reorder DECL_OVERRIDE_P check.

Index: gcc/cp/class.c
===================================================================
--- gcc/cp/class.c	(revision 274860)
+++ gcc/cp/class.c	(working copy)
@@ -2803,12 +2803,11 @@ get_basefndecls (tree name, tree t, vec<
 }
 
-/* If this declaration supersedes the declaration of
-   a method declared virtual in the base class, then
-   mark this field as being virtual as well.  */
+/* If this method overrides a virtual method from a base, then mark
+   this member function as being virtual as well.  Do 'final' and
+   'override' checks too.  */
 
 void
 check_for_override (tree decl, tree ctype)
 {
-  bool overrides_found = false;
   if (TREE_CODE (decl) == TEMPLATE_DECL)
     /* In [temp.mem] we have:
@@ -2817,15 +2816,19 @@ check_for_override (tree decl, tree ctyp
 	 override a virtual function from a base class.  */
     return;
-  if ((DECL_DESTRUCTOR_P (decl)
-       || IDENTIFIER_VIRTUAL_P (DECL_NAME (decl))
-       || DECL_CONV_FN_P (decl))
+
+  /* IDENTIFIER_VIRTUAL_P indicates whether the name has ever been
+     used for a vfunc.  That avoids the expensive
+     look_for_overrides call that when we know there's nothing to
+     find.  */
+  if (IDENTIFIER_VIRTUAL_P (DECL_NAME (decl))
       && look_for_overrides (ctype, decl)
+      /* Check staticness after we've checked if we 'override'.  */
       && !DECL_STATIC_FUNCTION_P (decl))
-    /* Set DECL_VINDEX to a value that is neither an INTEGER_CST nor
-       the error_mark_node so that we know it is an overriding
-       function.  */
     {
+      /* Set DECL_VINDEX to a value that is neither an INTEGER_CST nor
+	 the error_mark_node so that we know it is an overriding
+	 function.  */
       DECL_VINDEX (decl) = decl;
-      overrides_found = true;
+
       if (warn_override
 	  && !DECL_OVERRIDE_P (decl)
@@ -2835,10 +2838,16 @@ check_for_override (tree decl, tree ctyp
 		    "%qD can be marked override", decl);
     }
+  else if (DECL_OVERRIDE_P (decl))
+    error ("%q+#D marked %<override%>, but does not override", decl);
 
   if (DECL_VIRTUAL_P (decl))
     {
+      /* Remember this identifier is virtual name.  */
+      IDENTIFIER_VIRTUAL_P (DECL_NAME (decl)) = true;
+
       if (!DECL_VINDEX (decl))
+	/* It's a new vfunc.  */
 	DECL_VINDEX (decl) = error_mark_node;
-      IDENTIFIER_VIRTUAL_P (DECL_NAME (decl)) = 1;
+
       if (DECL_DESTRUCTOR_P (decl))
 	TYPE_HAS_NONTRIVIAL_DESTRUCTOR (ctype) = true;
@@ -2846,6 +2855,4 @@ check_for_override (tree decl, tree ctyp
   else if (DECL_FINAL_P (decl))
     error ("%q+#D marked %<final%>, but is not virtual", decl);
-  if (DECL_OVERRIDE_P (decl) && !overrides_found)
-    error ("%q+#D marked %<override%>, but does not override", decl);
 }
 

Re: [C++ PATCH] vfunc overrider simplification

[Nathan Sidwell]

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

On 8/23/19 3:24 PM, Nathan Sidwell wrote:
> In fixing a vfunc override bug on the modules branch, I noticed that 
> check_for_override can simply check IDENTIFIER_VIRTUAL_P -- the dtor 
> identifier and those for conversion operators will have it set 
> correctly.  (there a multiple conversion operator identifiers, but we 
> can only be overriding the one with the same return type, so that's fine.)

that turns out to be untrue -- the conversion operator table 
distinguishes between different typedefs of the same type.  This patch 
restores the DECL_CONV_FN_P test.

Applying to trunk.

nathan

-- 
Nathan Sidwell

[-- Attachment #2: vfn2.diff --]
[-- Type: text/x-patch, Size: 1648 bytes --]

2019-08-24  Nathan Sidwell  <nathan@acm.org>

	cp/
	* class.c (check_for_overrides): Conversion operators need
	checking too.

	testsuite/
	* g++.dg/inherit/virtual14.C: New.

Index: cp/class.c
===================================================================
--- cp/class.c	(revision 274902)
+++ cp/class.c	(working copy)
@@ -2817,10 +2817,12 @@ check_for_override (tree decl, tree ctyp
     return;
 
   /* IDENTIFIER_VIRTUAL_P indicates whether the name has ever been
-     used for a vfunc.  That avoids the expensive
-     look_for_overrides call that when we know there's nothing to
-     find.  */
-  if (IDENTIFIER_VIRTUAL_P (DECL_NAME (decl))
+     used for a vfunc.  That avoids the expensive look_for_overrides
+     call that when we know there's nothing to find.  As conversion
+     operators for the same type can have distinct identifiers, we
+     cannot optimize those in that way.  */
+  if ((IDENTIFIER_VIRTUAL_P (DECL_NAME (decl))
+       || DECL_CONV_FN_P (decl))
       && look_for_overrides (ctype, decl)
       /* Check staticness after we've checked if we 'override'.  */
       && !DECL_STATIC_FUNCTION_P (decl))
Index: testsuite/g++.dg/inherit/virtual14.C
===================================================================
--- testsuite/g++.dg/inherit/virtual14.C	(nonexistent)
+++ testsuite/g++.dg/inherit/virtual14.C	(working copy)
@@ -0,0 +1,24 @@
+// { dg-do run }
+
+struct base 
+{
+  virtual operator int () { return 0;}
+};
+
+typedef int q;
+
+struct d : base
+{
+  operator q () { return 1; }
+};
+
+int invoke (base *d)
+{
+  return int (*d);
+}
+
+int main ()
+{
+  d d;
+  return !(invoke (&d) == 1);
+}

Re: [C++ PATCH] vfunc overrider simplification

[Jason Merrill]

On Sat, Aug 24, 2019 at 6:43 PM Nathan Sidwell <nathan@acm.org> wrote:

> On 8/23/19 3:24 PM, Nathan Sidwell wrote:
> > In fixing a vfunc override bug on the modules branch, I noticed that
> > check_for_override can simply check IDENTIFIER_VIRTUAL_P -- the dtor
> > identifier and those for conversion operators will have it set
> > correctly.  (there a multiple conversion operator identifiers, but we
> > can only be overriding the one with the same return type, so that's
> fine.)
>
> that turns out to be untrue -- the conversion operator table
> distinguishes between different typedefs of the same type.


Hmm, that seems like a bug.

Jason

Re: [C++ PATCH] vfunc overrider simplification

[Nathan Sidwell]

On 8/25/19 2:35 PM, Jason Merrill wrote:
> On Sat, Aug 24, 2019 at 6:43 PM Nathan Sidwell <nathan@acm.org 
> <mailto:nathan@acm.org>> wrote:
> 
>     On 8/23/19 3:24 PM, Nathan Sidwell wrote:
>      > In fixing a vfunc override bug on the modules branch, I noticed that
>      > check_for_override can simply check IDENTIFIER_VIRTUAL_P -- the dtor
>      > identifier and those for conversion operators will have it set
>      > correctly.  (there a multiple conversion operator identifiers,
>     but we
>      > can only be overriding the one with the same return type, so
>     that's fine.)
> 
>     that turns out to be untrue -- the conversion operator table
>     distinguishes between different typedefs of the same type.
> 
> 
> Hmm, that seems like a bug.

It is by design.  That way we can propagate the form of the type to the 
function decl.

nathan

-- 
Nathan Sidwell