PosixPipes should not be copyable but should be movable.

[chaoren]

Summary

This addresses Oleksiy's comment in D9307.

[ovyalov]

include/lldb/Host/posix/PipePosix.h

33

We may need to extend PipeBase with moving constructor/assignment operator to make moving work for Pipe instances.

source/Host/posix/PipePosix.cpp

132–135

Nit: - you may use PipePosix(PipePosix::kInvalidDescriptor, PipePosix::kInvalidDescriptor)

[chaoren]

include/lldb/Host/posix/PipePosix.h

33

I'm probably not understanding this correctly. We can't have virtual constructors. I don't think a virtual assignment operator makes much sense either. Do you want a = default move consturctor/assignment operator for PipeBase? (Which does't make sense either, since there's nothing to move in PipeBase.)

source/Host/posix/PipePosix.cpp

132–135

Seems unnecessary. It's neither more concise, nor more efficient. I think I prefer the more straight forward initialization.

[ovyalov]

include/lldb/Host/posix/PipePosix.h

33

My understanding that moving doesn't work properly for derived classes unless all base types define move constructor and/or assignment operator.
I think there is no harm to declare move constructor/assignment operator for PipeBase as default even if it doesn't have member fields - if PipeBase will be extended with member fields, no further move changes will be needed.

[chaoren]

Future proof in case PipeBase ever gets extended with members fields.

[clayborg]

Looks fine

[ovyalov]

lgtm

include/lldb/Host/posix/PipePosix.h

 //----------------------------------------------------------------------
 class PipePosix : public PipeBase
 {
 public:
     static int kInvalidDescriptor;
 
     PipePosix();
     PipePosix(int read_fd, int write_fd);
+    PipePosix(const PipePosix &) = delete;
+    PipePosix(PipePosix &&pipe_posix);
+    PipePosix &operator=(const PipePosix &) = delete;
+    PipePosix &operator=(PipePosix &&pipe_posix);
 
     ~PipePosix() override;
 
     Error
     CreateNew(bool child_process_inherit) override;
     Error
     CreateNew(llvm::StringRef name, bool child_process_inherit) override;
     Error

source/Host/posix/PipePosix.cpp

while (!done)
         }
     }
     return error;
 }
 
 }
 
 PipePosix::PipePosix()
-    : m_fds{PipePosix::kInvalidDescriptor, PipePosix::kInvalidDescriptor} {}
+    : m_fds{
+        PipePosix::kInvalidDescriptor,
+        PipePosix::kInvalidDescriptor
+    } {}
 
 PipePosix::PipePosix(int read_fd, int write_fd)
     : m_fds{read_fd, write_fd} {}
 
+PipePosix::PipePosix(PipePosix &&pipe_posix)
+    : PipeBase{std::move(pipe_posix)},
+      m_fds{
+        pipe_posix.ReleaseReadFileDescriptor(),
+        pipe_posix.ReleaseWriteFileDescriptor()
+      } {}
+
+PipePosix &PipePosix::operator=(PipePosix &&pipe_posix)
+{
+    PipeBase::operator=(std::move(pipe_posix));
+    m_fds[READ] = pipe_posix.ReleaseReadFileDescriptor();
+    m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptor();
+    return *this;
+}
+
 PipePosix::~PipePosix()
 {
     Close();
 }
 
 Error
 PipePosix::CreateNew(bool child_processes_inherit)
 {

tools/lldb-server/lldb-gdbserver.cpp

 JoinListenThread ()
 {
     if (s_listen_thread.IsJoinable())
         s_listen_thread.Join(nullptr);
     return true;
 }
 
 Error
-WritePortToPipe(Pipe port_pipe, const uint16_t port)
+WritePortToPipe(Pipe &port_pipe, const uint16_t port)
 {
     char port_str[64];
     const auto port_str_len = ::snprintf(port_str, sizeof(port_str), "%u", port);
 
     size_t bytes_written = 0;
     // Write the port number as a C string with the NULL terminator.
     return port_pipe.Write(port_str, port_str_len + 1, bytes_written);
 }