Patchwork [C++] PR 49152

login
register
mail settings
Submitter Paolo Carlini
Date March 22, 2012, 12:22 a.m.
Message ID <4F6A70C5.7000109@oracle.com>
Download mbox | patch
Permalink /patch/148122/
State New
Headers show

Comments

Paolo Carlini - March 22, 2012, 12:22 a.m.
Hi,

this diagnostic issue is about not even trying to print expressions in 
error messages involving operators, and print operand types instead. 
Just as an example, for:

struct X { int x; };
void trigger (X x []) { x [01] = 0; }

we currently print:

error: no match for ‘operator=’ in ‘*(x + 4u) = 0’

which the patch changes to:

error: no match for ‘operator=’ (operand types are ‘X’ and ‘int’)

Or, for the existing other/error10.C, from:

error: no match for ‘operator-’ in ‘-(* & a)’

to

error: no match for ‘operator-’ (operand type is ‘A<0>’)

Jon and Manuel checked clang and I checked what icc does: without the 
caret, I don't think we can do *much* better here, but, wrt the audit 
trail discussion, I'm proposing printing the actual operand types 
between parentheses - I got the general idea from icc - because we don't 
want to confuse parameters and arguments.

Tested x86_64-linux.

Thanks,
Paolo.

/////////////////////////
/cp
2012-03-21  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/49152
	* call.c (op_error): Don't try to print expressions, print types.

/testsuite
2012-03-21  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/49152
	* g++.dg/diagnostic/operator1.C: New.
	* g++.dg/ext/label5.C: Adjust.
	* g++.dg/ext/va-arg1.C: Likewise.
	* g++.dg/other/error20.C: Likewise.
	* g++.dg/other/error20.C: Likewise.
	* g++.dg/other/error16.C: Likewise.
	* g++.dg/other/error10.C: Likewise.
	* g++.dg/parse/error30.C: Likewise.
	* g++.dg/cpp0x/lambda/lambda-err1.C: Likewise.
Gabriel Dos Reis - March 22, 2012, 4:25 a.m.
On Wed, Mar 21, 2012 at 7:22 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> this diagnostic issue is about not even trying to print expressions in error
> messages involving operators, and print operand types instead. Just as an
> example, for:
>
> struct X { int x; };
> void trigger (X x []) { x [01] = 0; }
>
> we currently print:
>
> error: no match for ‘operator=’ in ‘*(x + 4u) = 0’
>
> which the patch changes to:
>
> error: no match for ‘operator=’ (operand types are ‘X’ and ‘int’)
>
> Or, for the existing other/error10.C, from:
>
> error: no match for ‘operator-’ in ‘-(* & a)’
>
> to
>
> error: no match for ‘operator-’ (operand type is ‘A<0>’)
>

Usually these things appear in much less simpler expressions, possibly involving
the same symbol but withe different meanings.  There out be the a
way to give an indication of which symbol the diagnostic is about.
Withe the (imperfect) approach of printing expressions, at least some indication
is given on the expression involved.  Just printing the types with no indication
of what expression is causing trouble is more head-scratching.


> Jon and Manuel checked clang and I checked what icc does: without the caret,
> I don't think we can do *much* better here, but, wrt the audit trail

I think printing part or all of the expression is better in absence of carets.
We can improve on the pretty printing of expressions, for example.

> discussion, I'm proposing printing the actual operand types between
> parentheses - I got the general idea from icc - because we don't want to
> confuse parameters and arguments.
>
> Tested x86_64-linux.
>
> Thanks,
> Paolo.
>
> /////////////////////////
>
>
>
>
Paolo Carlini - March 22, 2012, 10:20 a.m.
Hi,

On 03/22/2012 05:25 AM, Gabriel Dos Reis wrote:
> On Wed, Mar 21, 2012 at 7:22 PM, Paolo Carlini<paolo.carlini@oracle.com>  wrote:
>> Hi,
>>
>> this diagnostic issue is about not even trying to print expressions in error
>> messages involving operators, and print operand types instead. Just as an
>> example, for:
>>
>> struct X { int x; };
>> void trigger (X x []) { x [01] = 0; }
>>
>> we currently print:
>>
>> error: no match for ‘operator=’ in ‘*(x + 4u) = 0’
>>
>> which the patch changes to:
>>
>> error: no match for ‘operator=’ (operand types are ‘X’ and ‘int’)
>>
>> Or, for the existing other/error10.C, from:
>>
>> error: no match for ‘operator-’ in ‘-(*&  a)’
>>
>> to
>>
>> error: no match for ‘operator-’ (operand type is ‘A<0>’)
>>
> Usually these things appear in much less simpler expressions, possibly involving
> the same symbol but withe different meanings.  There out be the a
> way to give an indication of which symbol the diagnostic is about.
> Withe the (imperfect) approach of printing expressions, at least some indication
> is given on the expression involved.  Just printing the types with no indication
> of what expression is causing trouble is more head-scratching.
Yes, you are right. Yesterday I had another look to the audit trail and 
thought, mistakenly, that people had a consensus about printing the 
types (by the way we are dealing also with much worse cases, like the 
one in Comment #0 there). Thus I don't think for the time being I can do 
much here, maybe the PR could be even closes as duplicate of the one 
about missing caret.

Thanks,
Paolo.
Marc Glisse - March 22, 2012, 4:13 p.m.
On Thu, 22 Mar 2012, Paolo Carlini wrote:

> Hi,
>
> On 03/22/2012 05:25 AM, Gabriel Dos Reis wrote:
>> On Wed, Mar 21, 2012 at 7:22 PM, Paolo Carlini<paolo.carlini@oracle.com> 
>> wrote:
>>> Hi,
>>> 
>>> this diagnostic issue is about not even trying to print expressions in 
>>> error
>>> messages involving operators, and print operand types instead. Just as an
>>> example, for:
>>> 
>>> struct X { int x; };
>>> void trigger (X x []) { x [01] = 0; }
>>> 
>>> we currently print:
>>> 
>>> error: no match for ‘operator=’ in ‘*(x + 4u) = 0’
>>> 
>>> which the patch changes to:
>>> 
>>> error: no match for ‘operator=’ (operand types are ‘X’ and ‘int’)
>>> 
>>> Or, for the existing other/error10.C, from:
>>> 
>>> error: no match for ‘operator-’ in ‘-(*&  a)’
>>> 
>>> to
>>> 
>>> error: no match for ‘operator-’ (operand type is ‘A<0>’)
>>> 
>> Usually these things appear in much less simpler expressions, possibly 
>> involving
>> the same symbol but withe different meanings.  There out be the a
>> way to give an indication of which symbol the diagnostic is about.
>> Withe the (imperfect) approach of printing expressions, at least some 
>> indication
>> is given on the expression involved.  Just printing the types with no 
>> indication
>> of what expression is causing trouble is more head-scratching.
> Yes, you are right. Yesterday I had another look to the audit trail and 
> thought, mistakenly, that people had a consensus about printing the types (by 
> the way we are dealing also with much worse cases, like the one in Comment #0 
> there). Thus I don't think for the time being I can do much here, maybe the 
> PR could be even closes as duplicate of the one about missing caret.

I haven't followed the whole diagnostic discussion, but what about 
printing both the reconstructed expression and the types?
Gabriel Dos Reis - March 22, 2012, 5 p.m.
On Thu, Mar 22, 2012 at 11:13 AM, Marc Glisse <marc.glisse@inria.fr> wrote:

> I haven't followed the whole diagnostic discussion, but what about printing
> both the reconstructed expression and the types?

Printing both isn't really the issue -- and we probably should.  (And I thought
we did in some cases.)

What is at issue is that the tree we print is one that is of a too
low-level form, sometimes
a bit removed from what user wrote (e.g. containing too much information to be
usefully processed by the user) therefore adding some confusion.  There are
several ways around this: (a) print the expression as written by the user
(this usually means dumping verbatim the expression from the input source file);
(b) reconstruct the expression from the tree but with a much higher level view
(this may be possible if the C++ front-end preserves certain properties from the
source code, but currently it does not.)

People usually refers to (a) as diagnostics with caret, because some popular
compilers dump the user input source file and use carets to delimit
the boundaries
of the expression under scrutiny.

-- Gaby


>
> --
> Marc Glisse
Paolo Carlini - April 16, 2012, 1:54 a.m.
.. hi all, hi Gaby,

a couple of days ago, Manuel suggested in the audit trail of the main 
"caret diagnostics" PR, that now that we actually have got a form of it, 
the kind of change I proposed to resolve PR 49152 may make much more 
sense. In any case, my original patch still regtests fine today.

What do you think?

Thanks,
Paolo.
Gabriel Dos Reis - April 16, 2012, 2:16 a.m.
On Sun, Apr 15, 2012 at 8:54 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> .. hi all, hi Gaby,
>
> a couple of days ago, Manuel suggested in the audit trail of the main "caret
> diagnostics" PR, that now that we actually have got a form of it, the kind
> of change I proposed to resolve PR 49152 may make much more sense. In any
> case, my original patch still regtests fine today.
>
> What do you think?

a hybrid approach; I would suggest something like this: (a) if caret
is in effect, then print
the caret pointing to the symbol in question; otherwise (b) print the
symbol and the type (as suggested by Marc).
This is all best abstracted in a separate function.
Marc Glisse - April 16, 2012, 5:42 a.m.
On Sun, 15 Apr 2012, Gabriel Dos Reis wrote:

> a hybrid approach; I would suggest something like this: (a) if caret
> is in effect, then print
> the caret pointing to the symbol in question; otherwise (b) print the
> symbol and the type (as suggested by Marc).

I may have forgotten the details, but looking at the beginning of the PR, 
don't we always want the types? Show where the failing operator== is, and 
print the types of the 2 arguments? I tried to parenthesize your sentence 
so it meant that, but it wasn't convincing...
Gabriel Dos Reis - April 16, 2012, 7:16 a.m.
On Mon, Apr 16, 2012 at 12:42 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Sun, 15 Apr 2012, Gabriel Dos Reis wrote:
>
>> a hybrid approach; I would suggest something like this: (a) if caret
>> is in effect, then print
>> the caret pointing to the symbol in question; otherwise (b) print the
>> symbol and the type (as suggested by Marc).
>
>
> I may have forgotten the details, but looking at the beginning of the PR,
> don't we always want the types?

Yes.

> Show where the failing operator== is, and
> print the types of the 2 arguments? I tried to parenthesize your sentence so
> it meant that, but it wasn't convincing...
>
> --
> Marc Glisse

Patch

Index: testsuite/g++.dg/ext/label5.C
===================================================================
--- testsuite/g++.dg/ext/label5.C	(revision 185603)
+++ testsuite/g++.dg/ext/label5.C	(working copy)
@@ -2,5 +2,5 @@ 
 // PR c++/24052
 
 struct A { };
-int main() { b: A() && && b; } // { dg-error "A\\(\\) && && *b" }
+int main() { b: A() && && b; } // { dg-error "operand types are 'A' and 'void\\*'" }
 // { dg-message "candidate|operator&&|no known conversion" "additional" { target *-*-* } 5 }
Index: testsuite/g++.dg/ext/va-arg1.C
===================================================================
--- testsuite/g++.dg/ext/va-arg1.C	(revision 185603)
+++ testsuite/g++.dg/ext/va-arg1.C	(working copy)
@@ -4,5 +4,5 @@  struct A {};
 
 void foo()
 {
-  ++__builtin_va_arg(0, A); // { dg-error "'\\+\\+va_arg\\(0, A\\)'" }
+  ++__builtin_va_arg(0, A); // { dg-error "operand type is 'A'" }
 }
Index: testsuite/g++.dg/other/error20.C
===================================================================
--- testsuite/g++.dg/other/error20.C	(revision 185603)
+++ testsuite/g++.dg/other/error20.C	(working copy)
@@ -8,6 +8,6 @@  struct A			// { dg-message "operator=|no known con
 
 void bar (A& a)
 {
-  a.foo () = 0; // { dg-error "A::foo\\(\\) = 0" }
+  a.foo () = 0; // { dg-error "operand types are 'A' and 'int'" }
   // { dg-message "candidate" "candidate note" { target *-*-* } 11 }
 }   
Index: testsuite/g++.dg/other/error16.C
===================================================================
--- testsuite/g++.dg/other/error16.C	(revision 185603)
+++ testsuite/g++.dg/other/error16.C	(working copy)
@@ -10,5 +10,5 @@  typedef Outer<X> XOuter;
 
 int main() {
   Outer<int>  ab;
-  ab.foo() == 1; // { dg-error "ab.Outer" }
+  ab.foo() == 1; // { dg-error "operand types are 'Outer<int>::Inner' and 'int'" }
 }
Index: testsuite/g++.dg/other/error10.C
===================================================================
--- testsuite/g++.dg/other/error10.C	(revision 185603)
+++ testsuite/g++.dg/other/error10.C	(working copy)
@@ -6,10 +6,9 @@  template<int> struct A {};
 
 template<int N>
 void foo(const A<N> &a)
-{ -A<N>(a); } // { dg-error "\\(\\* & a\\)" "" }
+{ -A<N>(a); } // { dg-error "operand type is 'A<0>'" }
 
 void bar()
 {
     foo(A<0>()); // { dg-message "required from here" "" }
 }
-
Index: testsuite/g++.dg/diagnostic/operator1.C
===================================================================
--- testsuite/g++.dg/diagnostic/operator1.C	(revision 0)
+++ testsuite/g++.dg/diagnostic/operator1.C	(revision 0)
@@ -0,0 +1,4 @@ 
+// PR c++/49152
+
+struct X { int x; }; 
+void trigger (X x []) { x [01] = 0; } // { dg-error "operand types are 'X' and 'int'" }
Index: testsuite/g++.dg/parse/error30.C
===================================================================
--- testsuite/g++.dg/parse/error30.C	(revision 185603)
+++ testsuite/g++.dg/parse/error30.C	(working copy)
@@ -8,5 +8,5 @@  struct A
   A(int);
 };
 
-A a = -A();	// { dg-error "10:no match for.*operator-.*in.*-A\\(\\)" }
-A b = -A(5);	// { dg-error "11:no match for.*operator-.*in.*-A\\(5\\)" }
+A a = -A();	// { dg-error "operand type is 'A'" }
+A b = -A(5);	// { dg-error "operand type is 'A'" }
Index: testsuite/g++.dg/cpp0x/lambda/lambda-err1.C
===================================================================
--- testsuite/g++.dg/cpp0x/lambda/lambda-err1.C	(revision 185603)
+++ testsuite/g++.dg/cpp0x/lambda/lambda-err1.C	(working copy)
@@ -4,5 +4,5 @@ 
 void foo()
 {
   int x[1];
-  [x]{} = 0;			// { dg-error "lambda closure" }
+  [x]{} = 0;			// { dg-error "lambda" }
 }
Index: cp/call.c
===================================================================
--- cp/call.c	(revision 185603)
+++ cp/call.c	(working copy)
@@ -4155,6 +4155,10 @@  op_error (enum tree_code code, enum tree_code code
 {
   const char *opname;
 
+  arg1 = arg1 ? TREE_TYPE (arg1) : NULL_TREE;
+  arg2 = arg2 ? TREE_TYPE (arg2) : NULL_TREE;
+  arg3 = arg3 ? TREE_TYPE (arg3) : NULL_TREE;
+
   if (code == MODIFY_EXPR)
     opname = assignment_operator_name_info[code2].name;
   else
@@ -4165,56 +4169,60 @@  op_error (enum tree_code code, enum tree_code code
     case COND_EXPR:
       if (match)
         error ("ambiguous overload for ternary %<operator?:%> "
-               "in %<%E ? %E : %E%>", arg1, arg2, arg3);
+	       "(operand types are %qT, %qT, and %qT)",
+	       arg1, arg2, arg3);
       else
         error ("no match for ternary %<operator?:%> "
-               "in %<%E ? %E : %E%>", arg1, arg2, arg3);
+               "(operand types are %qT, %qT, and %qT)",
+	       arg1, arg2, arg3);
       break;
 
     case POSTINCREMENT_EXPR:
     case POSTDECREMENT_EXPR:
       if (match)
-        error ("ambiguous overload for %<operator%s%> in %<%E%s%>",
-               opname, arg1, opname);
+        error ("ambiguous overload for %<operator%s%> "
+	       "(operand type is %qT)", opname, arg1);
       else
-        error ("no match for %<operator%s%> in %<%E%s%>", 
-               opname, arg1, opname);
+        error ("no match for %<operator%s%> "
+	       "(operand type is %qT)", opname, arg1);
       break;
 
     case ARRAY_REF:
       if (match)
-        error ("ambiguous overload for %<operator[]%> in %<%E[%E]%>", 
-               arg1, arg2);
+        error ("ambiguous overload for %<operator[]%> "
+	       "(operand types are %qT and %qT)", arg1, arg2);
       else
-        error ("no match for %<operator[]%> in %<%E[%E]%>", 
-               arg1, arg2);
+        error ("no match for %<operator[]%> "
+	       "(operand types are %qT and %qT)", arg1, arg2);
       break;
 
     case REALPART_EXPR:
     case IMAGPART_EXPR:
       if (match)
-        error ("ambiguous overload for %qs in %<%s %E%>", 
-               opname, opname, arg1);
+        error ("ambiguous overload for %qs (operand type is %qT)", 
+               opname, arg1);
       else
-        error ("no match for %qs in %<%s %E%>",
-               opname, opname, arg1);
+        error ("no match for %qs (operand type is %qT)",
+	       opname, arg1);
       break;
 
     default:
       if (arg2)
         if (match)
-          error ("ambiguous overload for %<operator%s%> in %<%E %s %E%>",
-                  opname, arg1, opname, arg2);
+          error ("ambiguous overload for %<operator%s%> "
+		 "(operand types are %qT and %qT)",
+		 opname, arg1, arg2);
         else
-          error ("no match for %<operator%s%> in %<%E %s %E%>",
-                 opname, arg1, opname, arg2);
+	  error ("no match for %<operator%s%> "
+		 "(operand types are %qT and %qT)",
+		 opname, arg1, arg2);
       else
         if (match)
-          error ("ambiguous overload for %<operator%s%> in %<%s%E%>",
-                 opname, opname, arg1);
+          error ("ambiguous overload for %<operator%s%> "
+		 "(operand type is %qT)", opname, arg1);
         else
-          error ("no match for %<operator%s%> in %<%s%E%>",
-                 opname, opname, arg1);
+          error ("no match for %<operator%s%> (operand type is %qT)",
+                 opname, arg1);
       break;
     }
 }