Re: [C++ Patch] PR 48630 (PR 31423)
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.
/
/cp
2011-10-21 Paolo Carlini paolo.carl...@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.carl...@oracle.com
PR c++/31423
* g++.dg/parse/error43.C: New.
* g++.dg/parse/error44.C: Likewise.
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)
OK. Jason
Re: [C++ Patch] PR 48630 (PR 31423)
On Fri, Oct 21, 2011 at 12:52 PM, Paolo Carlini paolo.carl...@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!
Re: [C++ Patch] PR 48630 (PR 31423)
On Wed, Oct 19, 2011 at 4:34 PM, Paolo Carlini paolo.carl...@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(Cc) {
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)
Surely we should only make this change for function members. Jason
Re: [C++ Patch] PR 48630 (PR 31423)
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.carl...@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.carl...@oracle.com PR c++/31423 PR c++/48630 * g++.dg/parse/error43.C: New. 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)
On Wed, Oct 19, 2011 at 6:36 PM, Paolo Carlini paolo.carl...@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)
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)
On Wed, Oct 19, 2011 at 7:09 PM, Paolo Carlini paolo.carl...@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(Cc) {
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)
On Wed, Oct 19, 2011 at 7:47 PM, Gabriel Dos Reis
g...@integrable-solutions.net wrote:
On Wed, Oct 19, 2011 at 7:09 PM, Paolo Carlini paolo.carl...@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(Cc) {
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)
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 likec.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(Cc) {
returnc.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.
