[Lldb-commits] [PATCH] D56234: [lldb-server] Add unnamed pipe support to PipeWindows
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Thu Jan 3 06:49:32 PST 2019
labath added inline comments.
Comment at: source/Host/windows/PipeWindows.cpp:28
+static std::string g_pipe_name_prefix = "\\\\.\\Pipe\\";
+} // namespace
This would be better as llvm::StringLiteral (or just a char array) to avoid global constructors. Also, `static` in an anonymous namespace is overkill (anon namespace implies static linkage).
Comment at: source/Host/windows/PipeWindows.cpp:47
+ // in the current process and usually crashes the program. Pass handles
+ // instead since the handle can be inheritated.
+ m_read = read_handle;
Comment at: source/Host/windows/PipeWindows.cpp:120
- // Open the write end of the pipe.
+ // FIXME: For server end pipe, it's not applicable to use CreateFile to get
+ // read or write end of the pipe for the reason that closing of the file will
I find it strange reading about "servers" in a low-level utility class that seemingly has nothing to do with serving. I think this will confuse future readers as they will not look at this in the context of this patch. Could you rephrase this?
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1074
+ auto write_handle = socket_pipe.GetWriteNativeHandle();
What do you think about adding `GetWriteNativeHandle` to the generic Pipe interface? In posix the file descriptor _is_ the native handle, so PipePosix could just return the fd, but it would allow you to write this bit of code generically without ifdefs (well, maybe except the `AppendCloseFileAction` below). Similarly, we could have a generic Pipe factory function(*) which takes the handles as input, which would allow you to implement the lldb-server side of things without ifdefs.
(*) I think a factory function (`Pipe::FromNativeHandles`?) would be better than a constructor to avoid ambiguities between descriptors and handles.
CHANGES SINCE LAST ACTION
More information about the lldb-commits