labath added inline comments.
================
Comment at: source/Host/windows/PipeWindows.cpp:28
std::atomic<uint32_t> g_pipe_serial(0);
-}
+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;
----------------
inherited
================
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();
+ debugserver_args.AppendArgument(llvm::StringRef("--pipe"));
----------------
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.
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56234/new/
https://reviews.llvm.org/D56234
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
