[Lldb-commits] [PATCH] D91634: [lldb] Error when there are no ports to launch a gdbserver on

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Looks good. Just some cosmetic details...



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h:30
 public:
-  using PortMap = std::map<uint16_t, lldb::pid_t>;
   using PacketHandler =
----------------
DavidSpickett wrote:
> Only the one in GDBRemoteCommunicationServerPlatform is/was used.
cool


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp:74
+    uint16_t port, lldb::pid_t pid) {
+  std::map<uint16_t, lldb::pid_t>::iterator pos = m_port_map.find(port);
+  if (pos != m_port_map.end()) {
----------------
`auto`, maybe


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h:29-30
+  public:
+    PortMap() = default;
+    PortMap(uint16_t min_port, uint16_t max_port);
+    // Add a port to the map. Does not change anything if it is already in the
----------------
Add some comments to explain what these do.


================
Comment at: lldb/tools/lldb-server/lldb-platform.cpp:234
       if (ch == 'P')
-        gdbserver_portmap[portnum] = LLDB_INVALID_PROCESS_ID;
+        gdbserver_portmap.AllowPort(static_cast<uint16_t>(portnum));
       else if (ch == 'm')
----------------
Why the cast?


================
Comment at: lldb/unittests/tools/lldb-server/tests/PortMapTest.cpp:1
+//===-- PortMapTest.cpp ---------------------------------------------------===//
+//
----------------
Please put this under `unittests/Process/gdb-remote`. The tests in here are a failed experiment, and I'll hopefully get rid of them soon.


================
Comment at: lldb/unittests/tools/lldb-server/tests/PortMapTest.cpp:24-25
+  llvm::Expected<uint16_t> available_port = p1.GetNextAvailablePort();
+  ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded());
+  ASSERT_EQ(0, *available_port);
+
----------------
ASSERT_THAT_EXPECTED(p1.GetNextAvailablePort(), llvm::HasValue(0));


================
Comment at: lldb/unittests/tools/lldb-server/tests/PortMapTest.cpp:33-34
+  available_port = p1.GetNextAvailablePort();
+  ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded());
+  ASSERT_EQ(1, *available_port);
+
----------------
Same, here, and further on...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91634/new/

https://reviews.llvm.org/D91634