r343285 - [cxx2a] P0641R2: (Some) type mismatches on defaulted functions only

[Richard Smith via cfe-commits]

On Tue, 2 Oct 2018 at 01:18, Stephan Bergmann via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> On 28/09/2018 03:16, Richard Smith via cfe-commits wrote:
> > Author: rsmith
> > Date: Thu Sep 27 18:16:43 2018
> > New Revision: 343285
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=343285&view=rev
> > Log:
> > [cxx2a] P0641R2: (Some) type mismatches on defaulted functions only
> > render the function deleted instead of rendering the program ill-formed.
> >
> > This change also adds an enabled-by-default warning for the case where
> > an explicitly-defaulted special member function of a non-template class
> > is implicitly deleted by the type checking rules. (This fires either due
> > to this language change or due to pre-C++20 reasons for the member being
> > implicitly deleted). I've tested this on a large codebase and found only
> > bugs (where the program means something that's clearly different from
> > what the programmer intended), so this is enabled by default, but we
> > should revisit this if there are problems with this being enabled by
> > default.
>
> Two questions on the new -Wdefaulted-function-deleted:
>
> 1  Do you have examples of actual bugs found by that?


What do you mean by "actual bugs"? Every instance I've seen it flag is a
case where the code means something other than what the programmer
intended. (Eg, every case fixed by https://reviews.llvm.org/rL343369 is a
bug in that sense.)
However, it's rare that it'll flag something that leads to the runtime
behavior of the program being different from the intent (usually you
instead get a later compile error in cases where that would happen). It is
possible, though -- in particular, it will flag cases where a move
operation that you really wanted to result in a move actually results in a
copy, and the program is silently accepted and is less efficient than you
had expected.


> I'm running it on
> the LibreOffice code base now and it feels like lots of just noise.  For
> example, I recently added explicitly defaulted decls of the four
> copy/move functions to lots of classes with a virtual dtor (where the
> dtor needs to be user-declared e.g. because it doesn't override any
> virtual base class dtor or because it shall not be defined inline), to
> silence GCC's new -Wdeprecated-copy.  If such a class then for example
> has a const non-static data member, Clang's -Wdefaulted-function-deleted
> now kicks in for the assignment ops.  I'm not sure whether it's better
> to have those functions explicitly deleted (which then needs revisiting
> when some details of the class change), or just explicitly defaulted and
> the compiler figure out whether its deleted or not (but which now causes
> Clang to bark), or simply not user-declared at all (but which now causes
> GCC to bark).
>

When revising (say) a non-copyable class results in it becoming copyable,
do you want that change to happen with no notice being given to the
programmer? I at least don't think that it's obvious that you do.

I'm fine with turning this warning off by default (maybe moving it to
-Wextra or similar) if there's a feeling that it's more noisy than it is
useful.

2  With
>
> > $ cat test.cc
> > struct S1 { S1 & operator =(S1 const &) = delete; };
> > struct S2 {
> >     S1 m;
> >     S2(S2 const &);
> > };
> > struct S3: S2 {
> >     S3 & operator =(S3 const &) = default;
> >     S3 & operator =(S3 &&) = default;
> > };
> >
> > $ clang++ -fsyntax-only test.cc
> > test.cc:7:10: warning: explicitly defaulted copy assignment operator is
> implicitly deleted [-Wdefaulted-function-deleted]
> >     S3 & operator =(S3 const &) = default;
> >          ^
> > test.cc:6:12: note: copy assignment operator of 'S3' is implicitly
> deleted because base class 'S2' has a deleted copy assignment operator
> > struct S3: S2 {
> >            ^
> > test.cc:3:8: note: copy assignment operator of 'S2' is implicitly
> deleted because field 'm' has a deleted copy assignment operator
> >     S1 m;
> >        ^
> > test.cc:1:18: note: 'operator=' has been explicitly marked deleted here
> > struct S1 { S1 & operator =(S1 const &) = delete; };
> >                  ^
> > test.cc:8:10: warning: explicitly defaulted move assignment operator is
> implicitly deleted [-Wdefaulted-function-deleted]
> >     S3 & operator =(S3 &&) = default;
> >          ^
> > test.cc:6:12: note: move assignment operator of 'S3' is implicitly
> deleted because base class 'S2' has a deleted move assignment operator
> > struct S3: S2 {
> >            ^
> > test.cc:3:8: note: copy assignment operator of 'S2' is implicitly
> deleted because field 'm' has a deleted copy assignment operator
> >     S1 m;
> >        ^
> > test.cc:1:18: note: 'operator=' has been explicitly marked deleted here
> > struct S1 { S1 & operator =(S1 const &) = delete; };
> >                  ^
> > 2 warnings generated.
>
> the final two notes for the second warning are somewhat confusing I
> think, as there's a mismatch from the preceding note's "'S2' has a
> deleted move assignment operator" to the second-last note's "copy
> assignment operator of 'S2' is implicitly deleted".  Not sure though
> whether this is an issue specific to this new
> -Wdefaulted-function-deleted, or a general issue with generic code
> producing such "deleted because" notes.
>

That's a general issue with the code that produces the "deleted because"
notes. The diagnostic text assumes that the same special member is the
relevant one in the source and destination (forgetting that a move can call
a copy). We should probably be saying that both the move *and* copy
assignment operators of 'S2' are deleted here (and explaining both), and
then explain why the move operation is deleted (although if the user
expected a fallback from move to copy, that also wouldn't be ideal).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181003/ebc0c9fb/attachment-0001.html>