labath added reviewers: jingham, davide. labath accepted this revision. labath added a comment. This revision is now accepted and ready to land.
The rationale you give for this class being in Target is sound from the POV of lldb client. (the CreateForHost function is slightly fishy, but not too much.) However, it starts to break down when you start to consider lldb-server, whose large chunk of responsibility revolves around signals, but it doesn't need anything else in the Target module. For that, I think the ultimate place of UnixSignals should be somewhere else (I was planning to try to move it to Utility, which would mean that GetHostSignals could stay where it is). However, we still have a long way to go before we can make lldb-server more independent, and the changes you make here are fairly small and easy to back out if needed. Since they solve the immediate problems with the Host module, I think it's fine to do this change now, and then revisit the placement of this class later. ================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp:401 - const auto &signals = Host::GetUnixSignals(); + lldb::UnixSignalsSP signals = UnixSignals::CreateForHost(); for (auto signo = signals->GetFirstSignalNumber(); ---------------- Despite being burried in `Plugins/Process/gdb-remote`, this code is actually part of lldb-server (and only lldb-server). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57780/new/ https://reviews.llvm.org/D57780 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
