[C++ Patch] PR 48630 (PR 31423)

[C++ Patch] PR 48630 (PR 31423)

[Paolo Carlini]

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

Hi,

in these two twin PRs filed by Ian and Gerald, it is pointed out that 
cases like:

    struct C {
      int f() { return 1; }
    };

    int f(C&c) {
      return ( 1 == c.f );
    }

where the user actually forgot the open-closed round braces are much 
more likely than cases where an ampersand is missing, still we output

invalid use of member (did you forget the ‘&’ ?)

Thus, the idea of saying instead

invalid use of member (did you forget the ‘()’ ?)

which I implemented in the patchlet below. Alternately we could give 
both hints, or refer to the argument list.

Tested x86_64-linux.

Thanks,
Paolo.

///////////////////

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

/cp
2011-10-19  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/31423
	PR c++/48630
	* typeck2.c (cxx_incomplete_type_diagnostic): Improve error message
	for invalid use of member.

/testsuite
2011-10-19  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/31423
	PR c++/48630
	* g++.dg/parse/error42.C: New.




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

Index: testsuite/g++.dg/parse/error42.C
===================================================================
--- testsuite/g++.dg/parse/error42.C	(revision 0)
+++ testsuite/g++.dg/parse/error42.C	(revision 0)
@@ -0,0 +1,5 @@
+// PR c++/48630
+// { dg-options "" }
+
+class C { public: C* f(); int get(); }; // { dg-error "forget the '\\(\\)'|base operand" }
+int f(C* p) { return p->f->get(); }
Index: cp/typeck2.c
===================================================================
--- cp/typeck2.c	(revision 180206)
+++ cp/typeck2.c	(working copy)
@@ -429,7 +429,7 @@ cxx_incomplete_type_diagnostic (const_tree value,
     case OFFSET_TYPE:
     bad_member:
       emit_diagnostic (diag_kind, input_location, 0,
-		       "invalid use of member (did you forget the %<&%> ?)");
+		       "invalid use of member (did you forget the %<()%> ?)");
       break;
 
     case TEMPLATE_TYPE_PARM:

Re: [C++ Patch] PR 48630 (PR 31423)

[Gabriel Dos Reis]

On Wed, Oct 19, 2011 at 4:34 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> in these two twin PRs filed by Ian and Gerald, it is pointed out that cases
> like:
>
>   struct C {
>     int f() { return 1; }
>   };
>
>   int f(C&c) {
>     return ( 1 == c.f );
>   }
>
> where the user actually forgot the open-closed round braces are much more
> likely than cases where an ampersand is missing, still we output
>
> invalid use of member (did you forget the ‘&’ ?)
>
> Thus, the idea of saying instead
>
> invalid use of member (did you forget the ‘()’ ?)
>
> which I implemented in the patchlet below. Alternately we could give both
> hints, or refer to the argument list.

I agree that '()' is a better default.  But we can do better.
We can use the type context, e.g. in initialization or
return-statement etc., to decide
whether () is intended (by looking at the TREE_TYPE of a member function type),
and only in cases where we can't tell we suggest both alternative:

     (did you intend a function call or address of non-static member function?)

Re: [C++ Patch] PR 48630 (PR 31423)

[Jason Merrill]

Surely we should only make this change for function members.

Jason

Re: [C++ Patch] PR 48630 (PR 31423)

[Paolo Carlini]

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

On 10/20/2011 12:32 AM, Jason Merrill wrote:
> Surely we should only make this change for function members.
Thanks Gaby and Jason. So, what about the below?

Tested x86_64-linux.

Paolo.

////////////////////

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

/cp
2011-10-19  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/31423
	PR c++/48630
	* typeck2.c (cxx_incomplete_type_diagnostic): Improve error message
	for invalid use of member function.

/testsuite
2011-10-19  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/31423
	PR c++/48630
	* g++.dg/parse/error43.C: New.




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

Index: cp/typeck2.c
===================================================================
--- cp/typeck2.c	(revision 180227)
+++ cp/typeck2.c	(working copy)
@@ -428,8 +428,14 @@ cxx_incomplete_type_diagnostic (const_tree value,
 
     case OFFSET_TYPE:
     bad_member:
-      emit_diagnostic (diag_kind, input_location, 0,
-		       "invalid use of member (did you forget the %<&%> ?)");
+      if (DECL_FUNCTION_MEMBER_P (TREE_OPERAND (value, 1)))
+	emit_diagnostic (diag_kind, input_location, 0,
+			 "invalid use of member function "
+			 "(did you forget the %<()%> ?)");
+      else
+	emit_diagnostic (diag_kind, input_location, 0,
+			 "invalid use of member "
+			 "(did you forget the %<&%> ?)");
       break;
 
     case TEMPLATE_TYPE_PARM:

Re: [C++ Patch] PR 48630 (PR 31423)

[Gabriel Dos Reis]

On Wed, Oct 19, 2011 at 6:36 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> On 10/20/2011 12:32 AM, Jason Merrill wrote:
>>
>> Surely we should only make this change for function members.
>
> Thanks Gaby and Jason. So, what about the below?

I believe the effect of your new patch is that if will
always emit the suggest "did you forget "()"?" for member functions,
even in the case where the current suggestion is correct.
Using the type context would prevent that regression.

If it unfortunate that we put this diagnostic in the same category
as real 'incomplete type usage.'  We should probably dissociate it.

Re: [C++ Patch] PR 48630 (PR 31423)

[Paolo Carlini]

On 10/20/2011 02:00 AM, Gabriel Dos Reis wrote:
> I believe the effect of your new patch is that if will
> always emit the suggest "did you forget "()"?" for member functions,
> even in the case where the current suggestion is correct.
> Using the type context would prevent that regression.
If you could give some guidance about the way to implement this, I may 
try over the next few days, otherwise probably I will have to give up 
for now (I assigned myself other PRs already), but it would be a pity, 
this PR has been reported 2 times by different people, apparently it's 
quite misleading. Anyway, I'm not assigned to the bug, even if I will 
not be able to actually help, it would be nice if you could attach to 
the audit trail a couple of nasty examples beyond what already 
considered in the analyses therein (both PRs)

Thanks!
Paolo.

Re: [C++ Patch] PR 48630 (PR 31423)

[Gabriel Dos Reis]

On Wed, Oct 19, 2011 at 7:09 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> On 10/20/2011 02:00 AM, Gabriel Dos Reis wrote:
>>
>> I believe the effect of your new patch is that if will
>> always emit the suggest "did you forget "()"?" for member functions,
>> even in the case where the current suggestion is correct.
>> Using the type context would prevent that regression.
>
> If you could give some guidance about the way to implement this, I may try
> over the next few days, otherwise probably I will have to give up for now (I
> assigned myself other PRs already), but it would be a pity, this PR has been
> reported 2 times by different people, apparently it's quite misleading.
> Anyway, I'm not assigned to the bug, even if I will not be able to actually
> help, it would be nice if you could attach to the audit trail a couple of
> nasty examples beyond what already considered in the analyses therein (both
> PRs)
>
> Thanks!
> Paolo.
>


I think I made a suggestion in my previous message:
  -- decouple this particular diagnostic from 'incomplete type' error.
      Because it has nothing to do with incomplete type error.

 -- once the diagnostic is decoupled, you could "grep" for all the
    places where cxx_incomplete_type_error or cxx_incomplete_type_diagnostic
    is called from.


My comments weren't in terms of "owenership" of the PR.

I would not necessarily say that they are nasty cases.

I know you don't like history but I believe it is important to
understand how the diagnostics came to be before fixing
them to prevent regressions -- or to purposefully break with
the past.

The reason why we have this suggestion in the first place is
because g++ supports the MS extension known as "bound member
function", e.g. something like &c.f, where c is an object and f is a
non-static member function.  It isn't ISO C++, but it is GNU C++
wth -fms-extensions.  A simple testcase is

struct C {
   int f() { return 1; }
   int g() { return 2; }
};

int f(C&c) {
   return &c.g == &c.f;
}


If you compile that program fragment with -fms-extensions, g++ will
accept it.  If you remove one of the '&', you get the diagnostic that
you want to fix.  You realize that if you use '()', you get another
type incompatible error, but not if you use '&' as suggested.

So the diagnostic could use both type context and test for -fms-extensions.

PS: more than a decade ago, I suggested removing this but people disagreed.

Re: [C++ Patch] PR 48630 (PR 31423)

[Gabriel Dos Reis]

On Wed, Oct 19, 2011 at 7:47 PM, Gabriel Dos Reis
<gdr@integrable-solutions.net> wrote:
> On Wed, Oct 19, 2011 at 7:09 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> On 10/20/2011 02:00 AM, Gabriel Dos Reis wrote:
>>>
>>> I believe the effect of your new patch is that if will
>>> always emit the suggest "did you forget "()"?" for member functions,
>>> even in the case where the current suggestion is correct.
>>> Using the type context would prevent that regression.
>>
>> If you could give some guidance about the way to implement this, I may try
>> over the next few days, otherwise probably I will have to give up for now (I
>> assigned myself other PRs already), but it would be a pity, this PR has been
>> reported 2 times by different people, apparently it's quite misleading.
>> Anyway, I'm not assigned to the bug, even if I will not be able to actually
>> help, it would be nice if you could attach to the audit trail a couple of
>> nasty examples beyond what already considered in the analyses therein (both
>> PRs)
>>
>> Thanks!
>> Paolo.
>>
>
>
> I think I made a suggestion in my previous message:
>  -- decouple this particular diagnostic from 'incomplete type' error.
>      Because it has nothing to do with incomplete type error.
>
>  -- once the diagnostic is decoupled, you could "grep" for all the
>    places where cxx_incomplete_type_error or cxx_incomplete_type_diagnostic
>    is called from.
>
>
> My comments weren't in terms of "owenership" of the PR.
>
> I would not necessarily say that they are nasty cases.
>
> I know you don't like history but I believe it is important to
> understand how the diagnostics came to be before fixing
> them to prevent regressions -- or to purposefully break with
> the past.
>
> The reason why we have this suggestion in the first place is
> because g++ supports the MS extension known as "bound member
> function", e.g. something like &c.f, where c is an object and f is a
> non-static member function.  It isn't ISO C++, but it is GNU C++
> wth -fms-extensions.  A simple testcase is
>
> struct C {
>   int f() { return 1; }
>   int g() { return 2; }
> };
>
> int f(C&c) {
>   return &c.g == &c.f;
> }
>
>
> If you compile that program fragment with -fms-extensions, g++ will
> accept it.  If you remove one of the '&', you get the diagnostic that
> you want to fix.  You realize that if you use '()', you get another
> type incompatible error, but not if you use '&' as suggested.
>
> So the diagnostic could use both type context and test for -fms-extensions.
>
> PS: more than a decade ago, I suggested removing this but people disagreed.
>

Another acceptable fix is to
  -- leave the current diagnostic as is if -fms-extensions
  -- suggest '()' is member function
  -- otherwise suggest '&'.

Re: [C++ Patch] PR 48630 (PR 31423)

[Paolo Carlini]

Hi,
> I think I made a suggestion in my previous message:
>   -- decouple this particular diagnostic from 'incomplete type' error.
>       Because it has nothing to do with incomplete type error.
>
>   -- once the diagnostic is decoupled, you could "grep" for all the
>     places where cxx_incomplete_type_error or cxx_incomplete_type_diagnostic
>     is called from.
>
>
> My comments weren't in terms of "owenership" of the PR.
>
> I would not necessarily say that they are nasty cases.
>
> I know you don't like history but I believe it is important to
> understand how the diagnostics came to be before fixing
> them to prevent regressions -- or to purposefully break with
> the past.
>
> The reason why we have this suggestion in the first place is
> because g++ supports the MS extension known as "bound member
> function", e.g. something like&c.f, where c is an object and f is a
> non-static member function.  It isn't ISO C++, but it is GNU C++
> wth -fms-extensions.  A simple testcase is
>
> struct C {
>    int f() { return 1; }
>    int g() { return 2; }
> };
>
> int f(C&c) {
>    return&c.g ==&c.f;
> }
>
>
> If you compile that program fragment with -fms-extensions, g++ will
> accept it.  If you remove one of the '&', you get the diagnostic that
> you want to fix.  You realize that if you use '()', you get another
> type incompatible error, but not if you use '&' as suggested.
>
> So the diagnostic could use both type context and test for -fms-extensions.
>
> PS: more than a decade ago, I suggested removing this but people disagreed.
>
> Another acceptable fix is to
>    -- leave the current diagnostic as is if -fms-extensions
>    -- suggest '()' is member function
>    -- otherwise suggest '&'.
Thanks for your help Gaby, in particular about the MS extension which I 
had overlooked completely (as any hard-code Linux guy would ;). Anyway, 
seriously, I'll try to come up with an improved proposal over the next days.

Thanks again,
Paolo.

Re: [C++ Patch] PR 48630 (PR 31423)

[Paolo Carlini]

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

Hi again,
>> Another acceptable fix is to
>>    -- leave the current diagnostic as is if -fms-extensions
>>    -- suggest '()' is member function
>>    -- otherwise suggest '&'.
> Thanks for your help Gaby, in particular about the MS extension which 
> I had overlooked completely (as any hard-code Linux guy would ;). 
> Anyway, seriously, I'll try to come up with an improved proposal over 
> the next days.
Thus I tested on x86_64-linux the below. Ok for mainline?

Thanks,
Paolo.

/////////////////////////

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

/cp
2011-10-21  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/31423
	* typeck2.c (cxx_incomplete_type_diagnostic): Improve error message
	for invalid use of member function.

/testsuite
2011-10-21  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/31423
	* g++.dg/parse/error43.C: New.
	* g++.dg/parse/error44.C: Likewise.

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

Index: testsuite/g++.dg/parse/error43.C
===================================================================
--- testsuite/g++.dg/parse/error43.C	(revision 0)
+++ testsuite/g++.dg/parse/error43.C	(revision 0)
@@ -0,0 +1,5 @@
+// PR c++/31423
+// { dg-options "" }
+
+class C { public: C* f(); int get(); };
+int f(C* p) { return p->f->get(); }  // { dg-error "forget the '\\(\\)'|base operand" }
Index: testsuite/g++.dg/parse/error44.C
===================================================================
--- testsuite/g++.dg/parse/error44.C	(revision 0)
+++ testsuite/g++.dg/parse/error44.C	(revision 0)
@@ -0,0 +1,11 @@
+// PR c++/31423
+// { dg-options "-fms-extensions" }
+
+struct C {
+   int f() { return 1; }
+   int g() { return 2; }
+};
+
+int f(C& c) {
+   return c.g == &c.f; // { dg-error "forget the '&'" }
+}
Index: cp/typeck2.c
===================================================================
--- cp/typeck2.c	(revision 180307)
+++ cp/typeck2.c	(working copy)
@@ -428,8 +428,15 @@ cxx_incomplete_type_diagnostic (const_tree value,
 
     case OFFSET_TYPE:
     bad_member:
-      emit_diagnostic (diag_kind, input_location, 0,
-		       "invalid use of member (did you forget the %<&%> ?)");
+      if (DECL_FUNCTION_MEMBER_P (TREE_OPERAND (value, 1))
+	  && ! flag_ms_extensions)
+	emit_diagnostic (diag_kind, input_location, 0,
+			 "invalid use of member function "
+			 "(did you forget the %<()%> ?)");
+      else
+	emit_diagnostic (diag_kind, input_location, 0,
+			 "invalid use of member "
+			 "(did you forget the %<&%> ?)");
       break;
 
     case TEMPLATE_TYPE_PARM:

Re: [C++ Patch] PR 48630 (PR 31423)

[Jason Merrill]

OK.

Jason

Re: [C++ Patch] PR 48630 (PR 31423)

[Gabriel Dos Reis]

On Fri, Oct 21, 2011 at 12:52 PM, Paolo Carlini
<paolo.carlini@oracle.com> wrote:
> Hi again,
>>>
>>> Another acceptable fix is to
>>>   -- leave the current diagnostic as is if -fms-extensions
>>>   -- suggest '()' is member function
>>>   -- otherwise suggest '&'.
>>
>> Thanks for your help Gaby, in particular about the MS extension which I
>> had overlooked completely (as any hard-code Linux guy would ;). Anyway,
>> seriously, I'll try to come up with an improved proposal over the next days.
>
> Thus I tested on x86_64-linux the below. Ok for mainline?

Yes, great!