C++ PATCH for c++/88136, -Wdeprecated-copy too noisy

-Wdeprecated-copy does find some real bugs, but it also complains
about a lot of reasonable code for which the implicitly declared copy
ctor/op= are fine oven though the class has a user-defined destructor:
this situation is only problematic if the destructor releases
resources held in one of the non-static data members.

So, this patch reins it in somewhat: first by moving from -Wall to
-Wextra, and then also only complaining if the other copy op is
user-declared.  The old behavior can be explicitly requested with
-Wdeprecated-copy-dtor.

Tested x86_64-pc-linux-gnu, applying to trunk.

On Thu, 6 Dec 2018 at 23:12, Jason Merrill <jason@redhat.com> wrote:
>
> -Wdeprecated-copy does find some real bugs, but it also complains
> about a lot of reasonable code for which the implicitly declared copy
> ctor/op= are fine oven though the class has a user-defined destructor:
> this situation is only problematic if the destructor releases
> resources held in one of the non-static data members.
>
> So, this patch reins it in somewhat: first by moving from -Wall to
> -Wextra, and then also only complaining if the other copy op is
> user-declared.  The old behavior can be explicitly requested with
> -Wdeprecated-copy-dtor.

Hmm.

g++ -c -pipe -O2 -fPIC -std=c++1z -fvisibility=hidden
-fvisibility-inlines-hidden -ffunction-sections -fdata-sections
-fno-exceptions -Wall -W -Wvla -Wdate-time -Wshift-overflow=2
-Wduplicated-cond -Wno-stringop-overflow -Werror -Wno-error=cpp
-Wno-error=deprecated-declarations -Wno-error=strict-overflow
-Wno-error=implicit-fallthrough -D_REENTRANT
-DQT_VERSION_STR='"5.12.0"' -DQT_VERSION_MAJOR=5 -DQT_VERSION_MINOR=12
-DQT_VERSION_PATCH=0 -DQT_BOOTSTRAPPED -DQT_NO_CAST_TO_ASCII
-DQT_NO_FOREACH -DQT_NO_CAST_FROM_ASCII
-DQT_NO_NARROWING_CONVERSIONS_IN_CONNECT -DQT_BUILD_BOOTSTRAP_LIB
-DQT_BUILDING_QT -DQT_ASCII_CAST_WARNINGS -DQT_MOC_COMPAT
-DQT_USE_QSTRINGBUILDER -DQT_DEPRECATED_WARNINGS
-DQT_DISABLE_DEPRECATED_BEFORE=0x050000 -DQT_NO_EXCEPTIONS
-D_LARGEFILE64_SOURCE -D_LARGEFILE_SOURCE -DQT_NO_DEBUG
-I/home/vivoutil/kuutti/qt5-5.12/qtbase/src/tools/bootstrap -I.
-I../../../include -I../../../include/QtCore
-I../../../include/QtCore/5.12.0
-I../../../include/QtCore/5.12.0/QtCore -I../../../include/QtXml
-I../../../include/QtXml/5.12.0 -I../../../include/QtXml/5.12.0/QtXml
-I/home/vivoutil/kuutti/qt5-5.12/qtbase/mkspecs/linux-g++ -o
.obj/qglobal.o /home/vivoutil/kuutti/qt5-5.12/qtbase/src/corelib/global/qglobal.cpp
In file included from ../../../include/QtCore/qvariant.h:1,
                 from
../../../include/QtCore/5.12.0/QtCore/private/../../../../../../../qtbase/src/corelib/tools/qlocale_p.h:58,
                 from
../../../include/QtCore/5.12.0/QtCore/private/../../../../../../../qtbase/src/corelib/tools/qlocale_tools_p.h:54,
                 from
../../../include/QtCore/5.12.0/QtCore/private/qlocale_tools_p.h:1,
                 from
/home/vivoutil/kuutti/qt5-5.12/qtbase/src/corelib/global/qglobal.cpp:52:
../../../include/QtCore/../../../../qtbase/src/corelib/kernel/qvariant.h:
In constructor ‘QVariant::QVariant(QVariant&&)’:
../../../include/QtCore/../../../../qtbase/src/corelib/kernel/qvariant.h:273:25:
error: implicitly-declared ‘constexpr QVariant::Private&
QVariant::Private::operator=(const QVariant::Private&)’ is deprecated
[-Werror=deprecated-copy]
  273 |     { other.d = Private(); }
      |                         ^
../../../include/QtCore/../../../../qtbase/src/corelib/kernel/qvariant.h:399:16:
note: because ‘QVariant::Private’ has user-provided
‘QVariant::Private::Private(const QVariant::Private&)’
  399 |         inline Private(const Private &other) Q_DECL_NOTHROW
      |                ^~~~~~~
cc1plus: all warnings being treated as errors

That doesn't have -Wextra. Yet the -Wdeprecated-copy still triggers.

On Sat, Dec 08, 2018 at 06:46:27PM +0200, Ville Voutilainen wrote:
> On Thu, 6 Dec 2018 at 23:12, Jason Merrill <jason@redhat.com> wrote:
> >
> > -Wdeprecated-copy does find some real bugs, but it also complains
> > about a lot of reasonable code for which the implicitly declared copy
> > ctor/op= are fine oven though the class has a user-defined destructor:
> > this situation is only problematic if the destructor releases
> > resources held in one of the non-static data members.
> >
> > So, this patch reins it in somewhat: first by moving from -Wall to
> > -Wextra, and then also only complaining if the other copy op is
> > user-declared.  The old behavior can be explicitly requested with
> > -Wdeprecated-copy-dtor.
> 
> Hmm.
> 
> g++ -c -pipe -O2 -fPIC -std=c++1z -fvisibility=hidden
> -fvisibility-inlines-hidden -ffunction-sections -fdata-sections
> -fno-exceptions -Wall -W -Wvla -Wdate-time -Wshift-overflow=2

-W is an alias to -Wextra.

> That doesn't have -Wextra. Yet the -Wdeprecated-copy still triggers.

	Jakub

On Sat, 8 Dec 2018 at 18:58, Jakub Jelinek <jakub@redhat.com> wrote:
> > g++ -c -pipe -O2 -fPIC -std=c++1z -fvisibility=hidden
> > -fvisibility-inlines-hidden -ffunction-sections -fdata-sections
> > -fno-exceptions -Wall -W -Wvla -Wdate-time -Wshift-overflow=2
>
> -W is an alias to -Wextra.

Yeah. Jason, I seem to have code that user-provides a copy constructor
(seemingly for no particular reason),
doesn't bother declaring a copy assignment operator, and still breaks
magnificently. :) There is no bug
in it; the assignment works as expected, so that's a false positive. I
am going to suggest taking this warning
out of -Wextra and making it completely separate for GCC 9.

On Sat, Dec 8, 2018 at 12:17 PM Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> On Sat, 8 Dec 2018 at 18:58, Jakub Jelinek <jakub@redhat.com> wrote:
> > > g++ -c -pipe -O2 -fPIC -std=c++1z -fvisibility=hidden
> > > -fvisibility-inlines-hidden -ffunction-sections -fdata-sections
> > > -fno-exceptions -Wall -W -Wvla -Wdate-time -Wshift-overflow=2
> >
> > -W is an alias to -Wextra.
>
> Yeah. Jason, I seem to have code that user-provides a copy constructor (seemingly for no particular reason),
> doesn't bother declaring a copy assignment operator, and still breaks magnificently. :) There is no bug
> in it; the assignment works as expected, so that's a false positive. I am going to suggest taking this warning
> out of -Wextra and making it completely separate for GCC 9.

The documented policy for -Wall is,

     This enables all the warnings about constructions that some users
     consider questionable, and that are easy to avoid (or modify to
     prevent the warning), even in conjunction with macros.
...
     Note that some warning flags are not implied by '-Wall'.  Some of
     them warn about constructions that users generally do not consider
     questionable, but which occasionally you might wish to check for;
     others warn about constructions that are necessary or hard to avoid
     in some cases, and there is no simple way to modify the code to
     suppress the warning.

It seems to me that this warning qualifies for -Wall under these
guidelines.  Providing a copy constructor without a matching
assignment operator is definitely suspect; the false positive only
comes in because, as you say, there was no good reason to provide the
copy constructor for Private.  And it's easy to avoid the warning by
declaring a defaulted assignment operator, if ABI concerns preclude
removing the constructor.

New compiler releases will usually include new warnings that require
some code modification to accommodate.  Why is this one particularly
problematic?

Jason

On Sat, 8 Dec 2018 at 19:53, Jason Merrill <jason@redhat.com> wrote:
> The documented policy for -Wall is,
>
>      This enables all the warnings about constructions that some users
>      consider questionable, and that are easy to avoid (or modify to
>      prevent the warning), even in conjunction with macros.
> ...
>      Note that some warning flags are not implied by '-Wall'.  Some of
>      them warn about constructions that users generally do not consider
>      questionable, but which occasionally you might wish to check for;
>      others warn about constructions that are necessary or hard to avoid
>      in some cases, and there is no simple way to modify the code to
>      suppress the warning.
>
> It seems to me that this warning qualifies for -Wall under these
> guidelines.  Providing a copy constructor without a matching
> assignment operator is definitely suspect; the false positive only
> comes in because, as you say, there was no good reason to provide the
> copy constructor for Private.  And it's easy to avoid the warning by
> declaring a defaulted assignment operator, if ABI concerns preclude
> removing the constructor.
>
> New compiler releases will usually include new warnings that require
> some code modification to accommodate.  Why is this one particularly
> problematic?

I don't think it's any more problematic than any other warning that
introduces new errors for fools that build with -Wall and -Werror.
But considering that those errors are false positives, and that
turning them off will in some cases require writing boiler-plate
(defaulted assignments), I would merely prefer having another release
round to get fixes for my codebase out in the wild.

On Sat, 8 Dec 2018 at 20:05, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:

> > New compiler releases will usually include new warnings that require
> > some code modification to accommodate.  Why is this one particularly
> > problematic?
>
> I don't think it's any more problematic than any other warning that
> introduces new errors for fools that build with -Wall and -Werror.
> But considering that those errors are false positives, and that
> turning them off will in some cases require writing boiler-plate
> (defaulted assignments), I would merely prefer having another release
> round to get fixes for my codebase out in the wild.

For what it's worth, I find it unfortunate that this deprecation and
its resulting warnings end up
making the decision on whether a "rule of 5" must be followed; correct
code needs to be adjusted
to cope with a fairly stylistic matter, with false positives and all.

I'll vote with my feet:

diff --git a/mkspecs/features/qt_common.prf b/mkspecs/features/qt_common.prf
index 4ad9946..3ba7eff 100644
--- a/mkspecs/features/qt_common.prf
+++ b/mkspecs/features/qt_common.prf
@@ -89,6 +89,8 @@ clang {
     greaterThan(QT_GCC_MAJOR_VERSION, 5): QMAKE_CXXFLAGS_WARN_ON +=
-Wshift-overflow=2 -Wduplicated-cond
     # GCC 7 has a lot of false positives relating to this, so disable
completely
     greaterThan(QT_GCC_MAJOR_VERSION, 6): QMAKE_CXXFLAGS_WARN_ON +=
-Wno-stringop-overflow
+    # GCC 9 has a lot of false positives relating to this, so disable
completely
+    greaterThan(QT_GCC_MAJOR_VERSION, 8): QMAKE_CXXFLAGS_WARN_ON +=
-Wno-deprecated-copy
 }

On Sat, Dec 8, 2018 at 1:33 PM Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> On Sat, 8 Dec 2018 at 20:05, Ville Voutilainen
> <ville.voutilainen@gmail.com> wrote:
>
> > > New compiler releases will usually include new warnings that require
> > > some code modification to accommodate.  Why is this one particularly
> > > problematic?
> >
> > I don't think it's any more problematic than any other warning that
> > introduces new errors for fools that build with -Wall and -Werror.
> > But considering that those errors are false positives, and that
> > turning them off will in some cases require writing boiler-plate
> > (defaulted assignments), I would merely prefer having another release
> > round to get fixes for my codebase out in the wild.
>
> For what it's worth, I find it unfortunate that this deprecation and its resulting warnings end up
> making the decision on whether a "rule of 5" must be followed; correct code needs to be adjusted
> to cope with a fairly stylistic matter, with false positives and all.

I don't see it as a stylistic matter.  If you need a user-provided
copy constructor to get proper copy semantics for a class, you almost
certainly need the same thing for copy assignment.  This was too noisy
for destructors, for which it's fairly common to define a virtual
destructor just to make a class polymorphic, not because there are
significant destruction semantics.  But I don't see a similar argument
for copy constructors: in your example, there was no need for
QVariant::Private to define a copy constructor, and that seems like a
situation where a warning is reasonable, even if the code is in fact
correct.

Jason

On Wed, 12 Dec 2018 at 16:52, Jason Merrill <jason@redhat.com> wrote:

> > For what it's worth, I find it unfortunate that this deprecation and its resulting warnings end up
> > making the decision on whether a "rule of 5" must be followed; correct code needs to be adjusted
> > to cope with a fairly stylistic matter, with false positives and all.
>
> I don't see it as a stylistic matter.  If you need a user-provided
> copy constructor to get proper copy semantics for a class, you almost
> certainly need the same thing for copy assignment.  This was too noisy
> for destructors, for which it's fairly common to define a virtual
> destructor just to make a class polymorphic, not because there are
> significant destruction semantics.  But I don't see a similar argument
> for copy constructors: in your example, there was no need for
> QVariant::Private to define a copy constructor, and that seems like a
> situation where a warning is reasonable, even if the code is in fact
> correct.

I have other cases where the compiler warns besides QVariant::Private.
I don't have a good grasp of what
those cases are like, yet, because I'll look at those warnings some
time later, possibly next year. :)
Thus I don't have any idea whether there are practical cases where the
copy constructor allocates but the assignment
can just blast bits, although I find that rather plausible. (Side
note: disabling warnings in Qt is painful; it's
not really more or less painful than doing code changes and regtesting
those on all platforms; that's not your
problem, though.)