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

login
register
mail settings
Submitter Paolo Carlini
Date Oct. 19, 2011, 11:36 p.m.
Message ID <4E9F5F0C.1050607@oracle.com>
Download mbox | patch
Permalink /patch/120708/
State New
Headers show

Comments

Paolo Carlini - Oct. 19, 2011, 11:36 p.m.
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.

////////////////////
/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.
Gabriel Dos Reis - Oct. 20, 2011, midnight
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.
Paolo Carlini - Oct. 20, 2011, 12:09 a.m.
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.
Gabriel Dos Reis - Oct. 20, 2011, 12:47 a.m.
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.
Gabriel Dos Reis - Oct. 20, 2011, 12:55 a.m.
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 '&'.
Paolo Carlini - Oct. 20, 2011, 1:03 a.m.
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.

Patch

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: