| Message ID | 4E9F5F0C.1050607@oracle.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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 '&'.
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.
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:
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.