| Message ID | 20200422063714.GA2295606@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Fix misuse of "override" in -Weffc++ warnings (PR 94698) | expand |
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
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
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).
On Wed, Apr 22, 2020 at 3:31 PM Jonathan Wakely <jwakely@redhat.com> 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
On 22/04/20 15:33 -0400, Jason Merrill wrote: >On Wed, Apr 22, 2020 at 3:31 PM Jonathan Wakely <jwakely@redhat.com> 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.
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 <jwakely@redhat.com> 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.
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 %<operator=(const %T&)%>", t); } else if (! TYPE_HAS_COPY_ASSIGN (t)) warning (OPT_Weffc__, - " but does not override %<operator=(const %T&)%>", t); + " but does not define %<operator=(const %T&)%>", t); inform (DECL_SOURCE_LOCATION (pointer_member), "pointer member %q+D declared here", pointer_member); }
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 <jwakely@redhat.com> 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.