[Lldb-commits] [PATCH] D22950: Centralize all calls to select() into a single class so we always call select properly

[Zachary Turner via lldb-commits]

zturner added a comment.

Currently running the test suite, will report back when it's done.


================
Comment at: include/lldb/Host/File.h:442
@@ -440,1 +441,3 @@
 
+    class SelectInfo
+    {
----------------
labath wrote:
> I am not sure about the location of this class, `File` seems like a bad place, especially considering that select does not even work with files (sockets only) on Windows. Maybe IOObject.h, or a standalone file?
+1 for making a new file out of this.  I would probably just call it `Select.h` or `SelectInfo.h`.  I like `Select` better than `SelectInfo` (as a file name and as a class name), because `SelectInfo` makes it sound like you're going to query the class for something, not that it's actually going to perform some system level operation.

================
Comment at: include/lldb/Host/File.h:493-498
@@ +492,8 @@
+
+            bool read_check;
+            bool write_check;
+            bool error_check;
+            bool read_ready;
+            bool write_ready;
+            bool error_ready;
+        };
----------------
How about a bitfield?

================
Comment at: include/lldb/Host/File.h:500
@@ +499,3 @@
+        };
+        std::map<int, FDInfo> m_fd_map;
+        std::unique_ptr<std::chrono::steady_clock::time_point> m_end_time_ap;
----------------
`llvm::DenseMap` will be more efficient (space-wise, and speed-wise)

================
Comment at: include/lldb/Host/File.h:501
@@ +500,3 @@
+        std::map<int, FDInfo> m_fd_map;
+        std::unique_ptr<std::chrono::steady_clock::time_point> m_end_time_ap;
+    };
----------------
labath wrote:
> I think we use `_up` as a suffix now.
`llvm::Optional<T>` is another option here.  It's nice in that it doesn't use a heap allocation and has better cache performance.

================
Comment at: source/Host/common/File.cpp:19
@@ -11,2 +18,2 @@
 
 #include <errno.h>
----------------
Inside of the `#if defined(_WIN32)` preprocessor section here, you need to add this line:

`#include <winsock2.h>`


https://reviews.llvm.org/D22950