[lmi] Is this class moveable or not? [Was: master 6a59da54 8/8: When in doubt, prefer the Rule of Five to the Rule of Zero]

[Greg Chicares]

On 7/12/22 21:40, Vadim Zeitlin wrote:
> On Tue, 12 Jul 2022 16:18:34 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> On 7/11/22 22:12, Vadim Zeitlin wrote:
> GC> > On Mon, 11 Jul 2022 16:22:14 +0000 Greg Chicares 
> <gchicares@sbcglobal.net> wrote:
[... can't snip the diff because it's referred to below ...]
> GC> > GC> > GC> diff --git a/gpt_server.hpp b/gpt_server.hpp
> GC> > GC> > GC> index cafeb177..f911087f 100644
> GC> > GC> > GC> --- a/gpt_server.hpp
> GC> > GC> > GC> +++ b/gpt_server.hpp
> GC> > GC> > GC> @@ -48,6 +48,12 @@ class LMI_SO gpt_server final
> GC> > GC> > GC>    public:
> GC> > GC> > GC>      explicit gpt_server(mcenum_emission);
> GC> > GC> > GC>  
> GC> > GC> > GC> +    gpt_server(gpt_server const&) = default;
> GC> > GC> > GC> +    gpt_server(gpt_server&&) = default;
> GC> > GC> > GC> +    gpt_server& operator=(gpt_server const&) = delete;
> GC> > GC> > GC> +    gpt_server& operator=(gpt_server&&) = delete;
> GC> > GC> > GC> +    ~gpt_server() = default;
> GC> > GC> > 
> GC> > GC> >  Just to show that I'm not going to comment negatively on all 
> changes in
> GC> > GC> > this commit, this one is perfectly fine: the class can be 
> copied/moved but
> GC> > GC> > not assigned to (in any way).
> GC> > GC> > 
> GC> > GC> >  Although, somewhat spoiling the effect I was looking for, I 
> think it might
> GC> > GC> > still be nice to explain why do we want to forbid assigning to 
> it...
> GC> 
> GC> I was just looking at that one, so let me share the question I'm
> GC> pondering. This class has a data member:
> GC>     gpt_state state_                 {};
> GC> of a class that has custom copy operations (because defaulting them
> GC> would not work), and I've never even thought of implementing move
> GC> operations there. Should gpt_server be non-movable because it contains
> GC> a non-movable member,
> 
>  I think it's already non-movable (which is something not immediately
> obvious from the diff above).

Interesting. The diff above, just to be clear, didn't attempt to suggest
whether the class should be movable or not; it's merely the result of:
 - writing all the move and copy special members with "= delete", and then
 - changing "= delete" to "= default" if the compiler complained.
I had taken that to mean that the move ctor was required. However...

Let's test that hypothesis by applying this patch to e998b52376e17, which
I pushed a couple hours ago:

--8<----8<----8<----8<----8<----8<----8<----8<----8<--
diff --git a/gpt_server.hpp b/gpt_server.hpp
index 524be42e2..f41f1ae96 100644
--- a/gpt_server.hpp
+++ b/gpt_server.hpp
@@ -49,9 +49,9 @@ class LMI_SO gpt_server final
     explicit gpt_server(mcenum_emission);
 
     gpt_server(gpt_server const&) = default;
-    gpt_server(gpt_server&&) = default;
-    gpt_server& operator=(gpt_server const&) = default;
-    gpt_server& operator=(gpt_server&&) = default;
+    gpt_server(gpt_server&&) = delete;
+    gpt_server& operator=(gpt_server const&) = delete;
+    gpt_server& operator=(gpt_server&&) = delete;
     ~gpt_server() = default;
 
     bool operator()(fs::path const&);
--8<----8<----8<----8<----8<----8<----8<----8<----8<--

MinGW-w64 gcc-10 compiles and links that, but GNU/Linux gcc-11 doesn't:

/usr/include/c++/11/bits/stl_algo.h: In instantiation of ‘constexpr _Funct 
std::for_each(_IIter, _IIter, _Funct) [with _IIter = 
__gnu_cxx::__normal_iterator<std::__cxx11::basic_string<char>*, 
std::vector<std::__cxx11::basic_string<char> > >; _Funct = gpt_server]’:
/opt/lmi/src/lmi/main_cli.cpp:502:10:   required from here
/usr/include/c++/11/bits/stl_algo.h:3821:14: error: use of deleted function 
‘gpt_server::gpt_server(gpt_server&&)’
 3821 |       return __f; // N.B. [alg.foreach] says std::move(f) but it's 
redundant.
      |              ^~~
In file included from /opt/lmi/src/lmi/main_cli.cpp:31:
/opt/lmi/src/lmi/gpt_server.hpp:52:5: note: declared here
   52 |     gpt_server(gpt_server&&) = delete;
      |     ^~~~~~~~~~

and neither does clang-13:

/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_algo.h:3821:14:
 error: call to deleted constructor of 'gpt_server'
      return __f; // N.B. [alg.foreach] says std::move(f) but it's redundant.
             ^~~
/opt/lmi/src/lmi/main_cli.cpp:498:10: note: in instantiation of function 
template specialization 
'std::for_each<__gnu_cxx::__normal_iterator<std::basic_string<char> *, 
std::vector<std::basic_string<char>>>, gpt_server>' requested here
    std::for_each
         ^
/opt/lmi/src/lmi/gpt_server.hpp:52:5: note: 'gpt_server' has been explicitly 
marked deleted here
    gpt_server(gpt_server&&) = delete;
    ^

That doesn't confirm the hypothesis that the move ctor is required,
or even that it would actually be used; it just says that I declared
it, so it participates in overload resolution, which prefers it,
but it was explicitly deleted. But now if I comment it out:

-    gpt_server(gpt_server&&) = delete;
+//  gpt_server(gpt_server&&) = delete;

then all compilers accept it. The original hypothesis is rejected:
the code can work with no move ctor, as long as we take care not to
declare it at all (which is not the same as declaring it as deleted).

But here's a surprise: what will the following patch print?
(Revert any earlier experimental change and apply it to HEAD.)
We speculated above that the class is non-movable, so our new
hypothesis is that it'll print "copied":

--8<----8<----8<----8<----8<----8<----8<----8<----8<--
diff --git a/gpt_server.hpp b/gpt_server.hpp
index 524be42e2..11ce53309 100644
--- a/gpt_server.hpp
+++ b/gpt_server.hpp
@@ -48,10 +48,10 @@ class LMI_SO gpt_server final
   public:
     explicit gpt_server(mcenum_emission);
 
-    gpt_server(gpt_server const&) = default;
-    gpt_server(gpt_server&&) = default;
-    gpt_server& operator=(gpt_server const&) = default;
-    gpt_server& operator=(gpt_server&&) = default;
+    gpt_server(gpt_server const&) {throw std::runtime_error("copied");}
+    gpt_server(gpt_server&&)      {throw std::runtime_error("moved" );}
+    gpt_server& operator=(gpt_server const&) = delete;
+    gpt_server& operator=(gpt_server&&) = delete;
     ~gpt_server() = default;
 
     bool operator()(fs::path const&);
--8<----8<----8<----8<----8<----8<----8<----8<----8<--

I built that uneventfully with GNU/Linux gcc-11, and tested it with
the following command (conveniently using a valid '.gpt' input file
that I just happened to have on hand, but one could be created
easily enough in the GUI:
  File | New | GPT ; OK ; File Save
and any valid file suffices):

/opt/lmi/bin[0]$lmipath=/opt/lmi/gcc_x86_64-pc-linux-gnu/build/ship; 
LD_LIBRARY_PATH="$lmipath:/opt/lmi/local/gcc_x86_64-pc-linux-gnu/lib/" 
$lmipath/lmi_cli_shared --ash_nazg --data_path=/opt/lmi/data --accept 
--file=/opt/lmi/test/a001.gpt 2>&1 |less

Exception type: 'std::runtime_error'
# 0 0x7f37407ffe16: /opt/lmi/src/lmi/unwind.cpp:199 __cxa_throw
# 1 0x55e4bf19702e: /opt/lmi/src/lmi/gpt_server.hpp:52 
process_command_line(int, char**) [clone .cold]
# 2 0x55e4bf19e0e9: /opt/lmi/src/lmi/main_cli.cpp:507 try_main(int, char**)
# 3 0x55e4bf197a7e: /opt/lmi/src/lmi/main_common_non_wx.cpp:48 main
# 4 0x7f373ffbd7fd: ../csu/libc-start.c:332 __libc_start_main
# 5 0x55e4bf197b9a: ???:0 _start
moved

Now, IIUC, our hypothesis was precisely that the move ctor would be
"defined as deleted" [class.copy.assign/7], but gcc-11 disagrees.
So does clang:

/opt/lmi/bin[0]$lmipath=/opt/lmi/clang_x86_64-pc-linux-gnu/build/ship; 
LD_LIBRARY_PATH="$lmipath:/opt/lmi/local/gcc_x86_64-pc-linux-gnu/lib/" 
$lmipath/lmi_cli_shared --ash_nazg --data_path=/opt/lmi/data --accept 
--file=/opt/lmi/test/a001.gpt          
Exception type: 'std::runtime_error'
what(): 'moved'
# 0 0x7fa27bbc582f: ???:0 __cxa_throw
# 1 0x20fd5d: ???:0 gpt_server::gpt_server(gpt_server&&)
# 2 0x20ebb8: ???:0 gpt_server std::for_each< [...large, merciful snip...]
# 3 0x20d8e5: ???:0 process_command_line(int, char**)
# 4 0x20e059: ???:0 try_main(int, char**)
# 5 0x211efb: ???:0 main
# 6 0x7fa27b07e7fd: ../csu/libc-start.c:332 __libc_start_main
# 7 0x20902a: ???:0 _start
moved

Does that falsify the hypothesis that this class cannot be moved?

> GC> or does the language quietly substitute copy semantics for that member
> GC> so that a defaulted move of the owning class is valid?

Here, I meant:

  for each data member M
     if M is moveable, move it
     else copy it

>  Yes, but perhaps not in the way you meant it: initializing gpt_server from
> an rvalue reference to gpt_server will use copy, but just because it will
> use its copy ctor. This happens because "= default" on the move constructor
> above actually _deletes_ it, because it can't be implemented, and such
> deleted-due-to-default move ctor is ignored by the overload resolution, so
> the copy ctor will be found and used instead.

Here, I think you're saying:

  if all data members are moveable, move them all
  else copy all data members

and that there exists one non-moveable data member, so the defaulted
move ctor is "defined as deleted" [class.copy.assign/7].

Does my demonstration above persuade you otherwise?
Or does it actually prove nothing, for some reason
that transcends my rather limited understanding?

>  Considering all this, I think it would be more clear to delete the move
> ctor instead. Alternatively, the class could be made actually movable,
> but this doesn't seem to be especially useful for this class.

Let's return to that last paragraph after resolving the last question above.