Re: [PATCH] c++: Fix misuse of "override" in -Weffc++ warnings (PR 94698)
On 22/04/20 20:43 +0100, Jonathan Wakely wrote:
On 22/04/20 15:33 -0400, Jason Merrill wrote:
On Wed, Apr 22, 2020 at 3:31 PM Jonathan Wakely wrote:
On 22/04/20 15:19 -0400, Jason Merrill wrote:
On 4/22/20 2:37 AM, Jonathan Wakely wrote:
These warnings have nothing to do with virtual functions, so "override"
is inappropriate. The warnings are just talking about defining special
members, so let's say that.
PR translation/94698
* class.c (check_field_decls): Change "override" to "define" in
-Weffc++ diagnostics.
Tested powerpc64le-linux, OK for master?
It is overriding the default(ed) definition, but I agree that
"override" now suggests virtual functions.
"define" is also wrong, though; it should be "declare". OK with that
change.
I did consider that, but decided that it has to be user-provided (i.e.
defined by the user) to avoid the problem, because a user-declared but
defaulted function would still not clean up pointer members.
True, but we don't warn in that case, and I think that's reasonable, as the
user is being explicit about their intent. Adding a defaulted declaration
seems like a good way to silence the warning without changing ABI.
Ah good point.
Fixed with this patch, committed to master.
commit cf88e25a2274f929d4789ca498fa3834836629c9
Author: Jonathan Wakely
Date: Wed Apr 22 23:18:06 2020 +0100
c++: Change -Weffc++ diagnostic to use "declare" (PR 94698)
Change the wording again, for the reasons given by Jason in
https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544362.html
PR translation/94698
* class.c (check_field_decls): Change "define" to "declare" in
-Weffc++ diagnostics.
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 6e14cd37aa4..e211db32377 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -3838,13 +3838,13 @@ check_field_decls (tree t, tree *access_decls,
if (! TYPE_HAS_COPY_CTOR (t))
{
warning (OPT_Weffc__,
- " but does not define %<%T(const %T&)%>", t, t);
+ " but does not declare %<%T(const %T&)%>", t, t);
if (!TYPE_HAS_COPY_ASSIGN (t))
warning (OPT_Weffc__, " or %", t);
}
else if (! TYPE_HAS_COPY_ASSIGN (t))
warning (OPT_Weffc__,
- " but does not define %", t);
+ " but does not declare %", t);
inform (DECL_SOURCE_LOCATION (pointer_member),
"pointer member %q+D declared here", pointer_member);
}
Re: [PATCH] c++: Fix misuse of "override" in -Weffc++ warnings (PR 94698)
On 22/04/20 15:33 -0400, Jason Merrill wrote: On Wed, Apr 22, 2020 at 3:31 PM Jonathan Wakely wrote: On 22/04/20 15:19 -0400, Jason Merrill wrote: >On 4/22/20 2:37 AM, Jonathan Wakely wrote: >>These warnings have nothing to do with virtual functions, so "override" >>is inappropriate. The warnings are just talking about defining special >>members, so let's say that. >> >> PR translation/94698 >> * class.c (check_field_decls): Change "override" to "define" in >> -Weffc++ diagnostics. >> >>Tested powerpc64le-linux, OK for master? > >It is overriding the default(ed) definition, but I agree that >"override" now suggests virtual functions. > >"define" is also wrong, though; it should be "declare". OK with that >change. I did consider that, but decided that it has to be user-provided (i.e. defined by the user) to avoid the problem, because a user-declared but defaulted function would still not clean up pointer members. True, but we don't warn in that case, and I think that's reasonable, as the user is being explicit about their intent. Adding a defaulted declaration seems like a good way to silence the warning without changing ABI. Ah good point.
Re: [PATCH] c++: Fix misuse of "override" in -Weffc++ warnings (PR 94698)
On Wed, Apr 22, 2020 at 3:31 PM Jonathan Wakely wrote: > On 22/04/20 15:19 -0400, Jason Merrill wrote: > >On 4/22/20 2:37 AM, Jonathan Wakely wrote: > >>These warnings have nothing to do with virtual functions, so "override" > >>is inappropriate. The warnings are just talking about defining special > >>members, so let's say that. > >> > >> PR translation/94698 > >> * class.c (check_field_decls): Change "override" to "define" in > >> -Weffc++ diagnostics. > >> > >>Tested powerpc64le-linux, OK for master? > > > >It is overriding the default(ed) definition, but I agree that > >"override" now suggests virtual functions. > > > >"define" is also wrong, though; it should be "declare". OK with that > >change. > > I did consider that, but decided that it has to be user-provided (i.e. > defined by the user) to avoid the problem, because a user-declared but > defaulted function would still not clean up pointer members. > True, but we don't warn in that case, and I think that's reasonable, as the user is being explicit about their intent. Adding a defaulted declaration seems like a good way to silence the warning without changing ABI. Jason
Re: [PATCH] c++: Fix misuse of "override" in -Weffc++ warnings (PR 94698)
On 22/04/20 15:19 -0400, Jason Merrill wrote: On 4/22/20 2:37 AM, Jonathan Wakely wrote: These warnings have nothing to do with virtual functions, so "override" is inappropriate. The warnings are just talking about defining special members, so let's say that. PR translation/94698 * class.c (check_field_decls): Change "override" to "define" in -Weffc++ diagnostics. Tested powerpc64le-linux, OK for master? It is overriding the default(ed) definition, but I agree that "override" now suggests virtual functions. "define" is also wrong, though; it should be "declare". OK with that change. I did consider that, but decided that it has to be user-provided (i.e. defined by the user) to avoid the problem, because a user-declared but defaulted function would still not clean up pointer members. But either seems better than "override". I'll change it to declared (I already committed the original version).
Re: [PATCH] c++: Fix misuse of "override" in -Weffc++ warnings (PR 94698)
On 4/22/20 2:37 AM, Jonathan Wakely wrote: These warnings have nothing to do with virtual functions, so "override" is inappropriate. The warnings are just talking about defining special members, so let's say that. PR translation/94698 * class.c (check_field_decls): Change "override" to "define" in -Weffc++ diagnostics. Tested powerpc64le-linux, OK for master? It is overriding the default(ed) definition, but I agree that "override" now suggests virtual functions. "define" is also wrong, though; it should be "declare". OK with that change. Jason
Re: [PATCH] c++: Fix misuse of "override" in -Weffc++ warnings (PR 94698)
On 4/22/20 2:37 AM, Jonathan Wakely via Gcc-patches wrote: These warnings have nothing to do with virtual functions, so "override" is inappropriate. The warnings are just talking about defining special members, so let's say that. PR translation/94698 * class.c (check_field_decls): Change "override" to "define" in -Weffc++ diagnostics. Tested powerpc64le-linux, OK for master? ok, thanks -- Nathan Sidwell
[PATCH] c++: Fix misuse of "override" in -Weffc++ warnings (PR 94698)
These warnings have nothing to do with virtual functions, so "override"
is inappropriate. The warnings are just talking about defining special
members, so let's say that.
PR translation/94698
* class.c (check_field_decls): Change "override" to "define" in
-Weffc++ diagnostics.
Tested powerpc64le-linux, OK for master?
commit fefa27c3514655e84c699245e282edfa552f9f64
Author: Jonathan Wakely
Date: Wed Apr 22 06:54:18 2020 +0100
c++: Fix misuse of "override" in -Weffc++ warnings (PR 94698)
These warnings have nothing to do with virtual functions, so "override"
is inappropriate. The warnings are just talking about defining special
members, so let's say that.
PR translation/94698
* class.c (check_field_decls): Change "override" to "define" in
-Weffc++ diagnostics.
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 41f52e5a5a0..6e14cd37aa4 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -3838,13 +3838,13 @@ check_field_decls (tree t, tree *access_decls,
if (! TYPE_HAS_COPY_CTOR (t))
{
warning (OPT_Weffc__,
- " but does not override %<%T(const %T&)%>", t, t);
+ " but does not define %<%T(const %T&)%>", t, t);
if (!TYPE_HAS_COPY_ASSIGN (t))
warning (OPT_Weffc__, " or %", t);
}
else if (! TYPE_HAS_COPY_ASSIGN (t))
warning (OPT_Weffc__,
-" but does not override %", t);
+" but does not define %", t);
inform (DECL_SOURCE_LOCATION (pointer_member),
"pointer member %q+D declared here", pointer_member);
}
