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

[Stephan Bergmann via cfe-commits]

On 04/10/2018 01:14, Richard Smith wrote:
> On Tue, 2 Oct 2018 at 01:18, Stephan Bergmann via cfe-commits 
> <cfe-commits at lists.llvm.org <mailto: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.)

You started to talk about "bugs" here :)  What I had a hard time 
figuring out is what such a case of =default would look like where the 
programmer's intend obviously had been that the function wouldn't be 
implicitly deleted.

> 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.

Thanks, that clarifies what you meant with "bugs".

>     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.

Having gone through all the warnings in the LibreOffice code base now, 
it wasn't that many individual warnings in the end (just blown up by 
being reported over and over again in the same include files), and the 
vast majority was involving classes that already exhibit pathological 
behavior anyway, like deriving from a base that has non-deleted copy 
ctor but deleted copy assignment op, due to somewhat broken overall 
design.  (Plus some cases where members had recently been made const, 
implicitly deleting the assignment ops.)  So I'm rather neutral now on 
whether that warning is on by default.

> 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.