Re: [C++ Patch] Fix confusing diagnostics for invalid overrides

[Nathan Sidwell]

On 03/27/2018 03:08 AM, Volker Reichelt wrote:

On 03/26/2018 01:19 PM, Nathan Sidwell wrote:

On 03/25/2018 10:18 AM, Volker Reichelt wrote:




How about the following then?
I rephrased the last three errors a little to really make them 
stand-alone errors.


Again, bootstrapped and regtested.
OK for trunk?


looks great, thanks!

nathan

--
Nathan Sidwell

Re: [C++ Patch] Fix confusing diagnostics for invalid overrides

[Volker Reichelt]

On 03/26/2018 01:19 PM, Nathan Sidwell wrote:

On 03/25/2018 10:18 AM, Volker Reichelt wrote:


Index: gcc/cp/search.c
===
--- gcc/cp/search.c    (revision 258835)
+++ gcc/cp/search.c    (working copy)
@@ -1918,12 +1918,14 @@
    if (fail == 1)
  {
    error ("invalid covariant return type for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "  overriding %q+#D", basefn);


In addtion to Paolo's comments, the new inform doesn't need the 
indentation.  Perhaps reword it to something like


"overridden function is %qD"

I.e. a more stand-alone message than a continuation of the error.

nathan


How about the following then?
I rephrased the last three errors a little to really make them 
stand-alone errors.


Again, bootstrapped and regtested.
OK for trunk?


2018-03-27  Volker Reichelt 

    * search.c (check_final_overrider): Use inform instead of error
    for the diagnostics of the overridden functions.  Tweak wording.

Index: gcc/cp/search.c
===
--- gcc/cp/search.c    (revision 258860)
+++ gcc/cp/search.c    (working copy)
@@ -1904,7 +1904,7 @@
   if (pedwarn (DECL_SOURCE_LOCATION (overrider), 0,
        "invalid covariant return type for %q#D", overrider))
     inform (DECL_SOURCE_LOCATION (basefn),
-            "  overriding %q#D", basefn);
+            "overridden function is %q#D", basefn);
 }
   else
 fail = 2;
@@ -1918,12 +1918,14 @@
   if (fail == 1)
 {
   error ("invalid covariant return type for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overridden function is %q#D", basefn);
 }
   else
 {
   error ("conflicting return type specified for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overridden function is %q#D", basefn);
 }
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
@@ -1938,7 +1940,8 @@
   if (!comp_except_specs (base_throw, over_throw, ce_derived))
 {
   error ("looser throw specifier for %q+#F", overrider);
-  error ("  overriding %q+#F", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn),
+      "overridden function is %q#F", basefn);
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
 }
@@ -1950,7 +1953,8 @@
   && !tx_safe_fn_type_p (over_type))
 {
   error ("conflicting type attributes specified for %q+#D", 
overrider);

-  error ("  overriding %q+#D", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn),
+      "overridden function is %q#D", basefn);
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
 }
@@ -1974,21 +1978,26 @@
 {
   if (DECL_DELETED_FN (overrider))
 {
-      error ("deleted function %q+D", overrider);
-      error ("overriding non-deleted function %q+D", basefn);
+      error ("deleted function %q+D overriding non-deleted function",
+         overrider);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overridden function is %qD", basefn);
   maybe_explain_implicit_delete (overrider);
 }
   else
 {
-      error ("non-deleted function %q+D", overrider);
-      error ("overriding deleted function %q+D", basefn);
+      error ("non-deleted function %q+D overriding deleted function",
+         overrider);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overridden function is %qD", basefn);
 }
   return 0;
 }
   if (DECL_FINAL_P (basefn))
 {
-  error ("virtual function %q+D", overrider);
-  error ("overriding final function %q+D", basefn);
+  error ("virtual function %q+D overriding final function", overrider);
+  inform (DECL_SOURCE_LOCATION (basefn),
+      "overridden function is %qD", basefn);
   return 0;
 }
   return 1;
===

2018-03-27  Volker Reichelt 

    * g++.dg/cpp0x/defaulted2.C: Use dg-message instead of dg-error
    for the diagnostics of overridden functions.  Adjust for new wording.
    * g++.dg/cpp0x/implicit1.C: Likewise.
    * g++.dg/cpp0x/override1.C: Likewise.
    * g++.dg/cpp1y/auto-fn18.C: Likewise.
    * g++.dg/eh/shadow1.C: Likewise.
    * g++.dg/inherit/covariant12.C: Likewise.
    * g++.dg/inherit/covariant14.C: Likewise.
    * g++.dg/inherit/covariant15.C: Likewise.
    * g++.dg/inherit/covariant16.C: Likewise.
    * g++.dg/inherit/crash3.C: Likewise.
    * g++.dg/inherit/error2.C: Likewise.
    * g++.dg/template/crash100.C: Likewise.
    * g++.old-deja/g++.eh/spec6.C: Likewise.
    * g++.old-deja/g++.mike/p811.C: Likewise.
    * g++.old-deja/g++.other/virtual11.C: Likewise.
    * g++.old-deja/g++.other/virtual4.C: 

Re: [C++ Patch] Fix confusing diagnostics for invalid overrides

[Nathan Sidwell]

On 03/25/2018 10:18 AM, Volker Reichelt wrote:


Index: gcc/cp/search.c
===
--- gcc/cp/search.c    (revision 258835)
+++ gcc/cp/search.c    (working copy)
@@ -1918,12 +1918,14 @@
    if (fail == 1)
  {
    error ("invalid covariant return type for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "  overriding %q+#D", basefn);


In addtion to Paolo's comments, the new inform doesn't need the 
indentation.  Perhaps reword it to something like


"overridden function is %qD"

I.e. a more stand-alone message than a continuation of the error.

nathan
--
Nathan Sidwell

Re: [C++ Patch] Fix confusing diagnostics for invalid overrides

[Volker Reichelt]

On 03/25/2018 08:48 PM, Paolo Carlini wrote:

Hi Volker


On 25 Mar 2018, at 16:18, Volker Reichelt  wrote:

Hi,

when overriding a virtual function fails, the C++ front-end usually
emits two errors: one for the override that fails and one for the
function that is overridden. The second error is confusing and
should be replaced by a note to be in line with other diagnostics.

The attached patch just does that: It replaces the error/error pattern
by an error/inform pattern in search.c (check_final_overrider). And it
replaces the affected dg-error marks in the testsuite by dg-message.

Since you are correctly using DECL_SOURCE_LOCATION, I would recommend removing 
the ‘+’ modifiers, which in that case are redundant anyway and are known be a 
little cryptic and even causing tricky bugs together with warnings.

Cheers,
Paolo

Thanks, Paolo.
Below is an updated patch without the redundant "+" modifiers.

On 03/26/2018 12:33 AM, Paolo Carlini wrote:

On 25/03/2018 21:08, Paolo Carlini wrote:
... oh, please also double check that with 'F' you don't need the 
general location_of instead of D_S_L, which normally goes with 'D' - 
I don't have my machines at hand to do it myself, sorry.

Just checked, DECL_SOURCE_LOCATION is fine.

Paolo.


I also checked that DECL_SOURCE_LOCATION with %qF is OK.
In all the other places in the C++ frontend where %q#F or %qF is
used, DECL_SOURCE_LOCATION() is used for the corresponding argument:

  call.c (joust)
  decl.c (wrapup_namespace_globals)
  decl.c (check_redeclaration_exception_specification)
  method.c (maybe_explain_implicit_delete)

I also checked the output for that specific case manually.

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker


2018-03-25  Volker Reichelt  

    * search.c (check_final_overrider): Use inform instead of error
    for the diagnostics of the overridden functions.

Index: gcc/cp/search.c
===
--- gcc/cp/search.c    (revision 258835)
+++ gcc/cp/search.c    (working copy)
@@ -1918,12 +1918,12 @@
   if (fail == 1)
 {
   error ("invalid covariant return type for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn), "  overriding %q#D", basefn);
 }
   else
 {
   error ("conflicting return type specified for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn), "  overriding %q#D", basefn);
 }
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
@@ -1938,7 +1938,7 @@
   if (!comp_except_specs (base_throw, over_throw, ce_derived))
 {
   error ("looser throw specifier for %q+#F", overrider);
-  error ("  overriding %q+#F", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn), "  overriding %q#F", basefn);
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
 }
@@ -1950,7 +1950,7 @@
   && !tx_safe_fn_type_p (over_type))
 {
   error ("conflicting type attributes specified for %q+#D", 
overrider);

-  error ("  overriding %q+#D", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn), "  overriding %q#D", basefn);
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
 }
@@ -1975,13 +1975,15 @@
   if (DECL_DELETED_FN (overrider))
 {
   error ("deleted function %q+D", overrider);
-      error ("overriding non-deleted function %q+D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overriding non-deleted function %qD", basefn);
   maybe_explain_implicit_delete (overrider);
 }
   else
 {
   error ("non-deleted function %q+D", overrider);
-      error ("overriding deleted function %q+D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overriding deleted function %qD", basefn);
 }
   return 0;
 }
@@ -1988,7 +1990,8 @@
   if (DECL_FINAL_P (basefn))
 {
   error ("virtual function %q+D", overrider);
-  error ("overriding final function %q+D", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn),
+      "overriding final function %qD", basefn);
   return 0;
 }
   return 1;
===

2018-03-25  Volker Reichelt  

    * g++.dg/cpp0x/defaulted2.C: Use dg-message instead of dg-error
    for the diagnostics of overridden functions.
    * g++.dg/cpp0x/implicit1.C: Likewise.
    * g++.dg/cpp0x/override1.C: Likewise.
    * g++.dg/eh/shadow1.C: Likewise.
    * g++.dg/inherit/covariant12.C: Likewise.
    * g++.dg/inherit/covariant14.C: Likewise.
    * g++.dg/inherit/covariant15.C: Likewise.
    * g++.dg/inherit/covariant16.C: Likewise.
    * g++.dg/inherit/crash3.C: Likewise.
    * g++.dg/inherit/error2.C: Likewise.
    * g++.dg/template/crash100.C: Likewise.
    * g++.old-deja/g++.eh/spec6.C: Likewise.
    * 

Re: [C++ Patch] Fix confusing diagnostics for invalid overrides

[Paolo Carlini]

On 25/03/2018 21:08, Paolo Carlini wrote:

... oh, please also double check that with 'F' you don't need the general 
location_of instead of D_S_L, which normally goes with 'D' - I don't have my 
machines at hand to do it myself, sorry.

Just checked, DECL_SOURCE_LOCATION is fine.

Paolo.

Re: [C++ Patch] Fix confusing diagnostics for invalid overrides

[Paolo Carlini]

... oh, please also double check that with 'F' you don't need the general 
location_of instead of D_S_L, which normally goes with 'D' - I don't have my 
machines at hand to do it myself, sorry.

Paolo

Re: [C++ Patch] Fix confusing diagnostics for invalid overrides

[Paolo Carlini]

Hi Volker

> On 25 Mar 2018, at 16:18, Volker Reichelt  wrote:
> 
> Hi,
> 
> when overriding a virtual function fails, the C++ front-end usually
> emits two errors: one for the override that fails and one for the
> function that is overridden. The second error is confusing and
> should be replaced by a note to be in line with other diagnostics.
> 
> The attached patch just does that: It replaces the error/error pattern
> by an error/inform pattern in search.c (check_final_overrider). And it
> replaces the affected dg-error marks in the testsuite by dg-message.

Since you are correctly using DECL_SOURCE_LOCATION, I would recommend removing 
the ‘+’ modifiers, which in that case are redundant anyway and are known be a 
little cryptic and even causing tricky bugs together with warnings.

Cheers,
Paolo

[C++ Patch] Fix confusing diagnostics for invalid overrides

[Volker Reichelt]

Hi,

when overriding a virtual function fails, the C++ front-end usually
emits two errors: one for the override that fails and one for the
function that is overridden. The second error is confusing and
should be replaced by a note to be in line with other diagnostics.

The attached patch just does that: It replaces the error/error pattern
by an error/inform pattern in search.c (check_final_overrider). And it
replaces the affected dg-error marks in the testsuite by dg-message.

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker


2018-03-25  Volker Reichelt 

    * search.c (check_final_overrider): Use inform instead of error
    for the diagnostics of the overridden functions.

Index: gcc/cp/search.c
===
--- gcc/cp/search.c    (revision 258835)
+++ gcc/cp/search.c    (working copy)
@@ -1918,12 +1918,14 @@
   if (fail == 1)
 {
   error ("invalid covariant return type for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "  overriding %q+#D", basefn);
 }
   else
 {
   error ("conflicting return type specified for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "  overriding %q+#D", basefn);
 }
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
@@ -1938,7 +1940,7 @@
   if (!comp_except_specs (base_throw, over_throw, ce_derived))
 {
   error ("looser throw specifier for %q+#F", overrider);
-  error ("  overriding %q+#F", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn), "  overriding %q+#F", basefn);
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
 }
@@ -1950,7 +1952,7 @@
   && !tx_safe_fn_type_p (over_type))
 {
   error ("conflicting type attributes specified for %q+#D", 
overrider);

-  error ("  overriding %q+#D", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn), "  overriding %q+#D", basefn);
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
 }
@@ -1975,13 +1977,15 @@
   if (DECL_DELETED_FN (overrider))
 {
   error ("deleted function %q+D", overrider);
-      error ("overriding non-deleted function %q+D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overriding non-deleted function %q+D", basefn);
   maybe_explain_implicit_delete (overrider);
 }
   else
 {
   error ("non-deleted function %q+D", overrider);
-      error ("overriding deleted function %q+D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overriding deleted function %q+D", basefn);
 }
   return 0;
 }
@@ -1988,7 +1992,8 @@
   if (DECL_FINAL_P (basefn))
 {
   error ("virtual function %q+D", overrider);
-  error ("overriding final function %q+D", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn),
+      "overriding final function %q+D", basefn);
   return 0;
 }
   return 1;
===

2018-03-25  Volker Reichelt 

    * g++.dg/cpp0x/defaulted2.C: Use dg-message instead of dg-error
    for the diagnostics of overridden functions.
    * g++.dg/cpp0x/implicit1.C: Likewise.
    * g++.dg/cpp0x/override1.C: Likewise.
    * g++.dg/eh/shadow1.C: Likewise.
    * g++.dg/inherit/covariant12.C: Likewise.
    * g++.dg/inherit/covariant14.C: Likewise.
    * g++.dg/inherit/covariant15.C: Likewise.
    * g++.dg/inherit/covariant16.C: Likewise.
    * g++.dg/inherit/crash3.C: Likewise.
    * g++.dg/inherit/error2.C: Likewise.
    * g++.dg/template/crash100.C: Likewise.
    * g++.old-deja/g++.eh/spec6.C: Likewise.
    * g++.old-deja/g++.mike/p811.C: Likewise.
    * g++.old-deja/g++.other/virtual11.C: Likewise.
    * g++.old-deja/g++.other/virtual4.C: Likewise.

Index: gcc/testsuite/g++.dg/cpp0x/defaulted2.C
===
--- gcc/testsuite/g++.dg/cpp0x/defaulted2.C    (revision 258835)
+++ gcc/testsuite/g++.dg/cpp0x/defaulted2.C    (working copy)
@@ -25,7 +25,7 @@

 struct C
 {
-  virtual void f() = delete;    // { dg-error "overriding deleted" }
+  virtual void f() = delete;    // { dg-message "overriding deleted" }
 };

 struct D: public C
Index: gcc/testsuite/g++.dg/cpp0x/implicit1.C
===
--- gcc/testsuite/g++.dg/cpp0x/implicit1.C    (revision 258835)
+++ gcc/testsuite/g++.dg/cpp0x/implicit1.C    (working copy)
@@ -7,7 +7,7 @@
 {
   void operator delete (void *); // { dg-message "private" }
 public:
-  virtual ~C();            // { dg-error "overriding" }
+  virtual ~C();            // { dg-message "overriding" }
 };

 struct D: C { };        // { dg-error "deleted" }
@@ -20,7 +20,7 @@

 struct F
 {
-  virtual ~F();            // { dg-error "overriding" }
+  virtual