[Lldb-commits] [PATCH] Enhance the Pipe interface for better portability
zturner at google.com
Tue Dec 16 14:17:59 PST 2014
Comment at: include/lldb/Host/PipeBase.h:27
@@ +26,3 @@
+ virtual Error CreateNew(bool child_process_inherit) = 0;
+ virtual Error CreateNew(bool child_process_inherit, llvm::StringRef name) = 0;
+ virtual Error OpenAsReader(bool child_process_inherit, llvm::StringRef name) = 0;
> Minor thing - you may have name passed as a first parameter so a signature will be in accord with open sys call syntax - open(name, flags)
Comment at: source/Host/posix/PipePosix.cpp:76
@@ -68,2 +75,3 @@
- return false;
+ return error;
> Could you call error.SetErrorToErrno(); before Close() to make sure that Close doesn't overwrite errno?
Yea, good point. I'll fix this.
Comment at: source/Host/windows/PipeWindows.cpp:116
@@ -78,3 +115,3 @@
- m_read_overlapped = new OVERLAPPED;
- ZeroMemory(m_read_overlapped, sizeof(OVERLAPPED));
+ m_read = ::CreateFile(name.data(), GENERIC_READ, 0, &attributes, OPEN_EXISTING, FILE_FLAG_OVERLAPPED, NULL);
+ if (INVALID_HANDLE_VALUE == m_read)
> Just out of curiosity - why are you using std::string::data here, not c_str?
This isn't std::string::data, it's llvm::StringRef::data. llvm::StringRef is a wrapper around a char *, but it's immutable and doesn't own the string data. It just provides convenience methods like length(), equals_lower(), etc.
However, it doesn't provide a c_str() method. The reason for that is because StringRef can be used to refer to non-null terminated string data. So it stores the length internally. In theory they could provide a c_str() method that's the same as the data() method, but they argue that people will be confused and assume that the result of c_str() is null terminated. If you create the StringRef yourself, and you know it refers to a null terminated string, then it's safe to treat data() as a null terminated string.
More information about the lldb-commits