[Lldb-commits] [PATCH] Enhance the Pipe interface for better portability

[Zachary Turner]

================
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;
----------------
ovyalov wrote:
> 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) 
Sounds reasonable.

================
Comment at: source/Host/posix/PipePosix.cpp:76
@@ -68,2 +75,3 @@
                 Close();
-                return false;
+                error.SetErrorToErrno();
+                return error;
----------------
ovyalov wrote:
> 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)
----------------
ovyalov wrote:
> 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.

http://reviews.llvm.org/D6686

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/