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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits