[Lldb-commits] [PATCH] D30286: Implement QPassSignals GDB package in lldb-server

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Good stuff. We just need a couple of changes to make this conform better with llvm guidelines.



================
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:72
+  //------------------------------------------------------------------
+  virtual Error IgnoreSignals(const std::vector<int> &signals);
+
----------------
llvm::ArrayRef<int> signals


================
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:320
+  // without stopping it.
+  std::unordered_set<int> m_signals_to_ignore;
+
----------------
use of unordered_set is discouraged in llvm <http://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc>. It sounds like this would be a good use-case for a llvm::DenseSet or potentially SmallSet.


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/signal-filtering/Makefile:3
+
+CFLAGS_EXTRAS += -D__STDC_LIMIT_MACROS -D__STDC_FORMAT_MACROS -std=c++11
+CXX_SOURCES := main.cpp
----------------
I am pretty sure the CFLAGS_EXTRAS line is not necessary here.


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/signal-filtering/TestGdbRemote_QPassSignals.py:73
+    @llgs_test
+    def test_default_signals_behavior(self):
+        self.init_llgs_test()
----------------
I really like how these tests are written. May suggest one more improvement:
- have the inferior count how many signals it received and then return that number from main()
- in the test you can then check the exit code matches the number of signals you are expecting
This should verify that we actually pass the signals and not just swallow them silently.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3097
+      break; // End of string
+    }
+    if (separator != ';') {
----------------
This is not a big deal, but in LLVM we generally don't put braces around blocks that fit on a single line.


https://reviews.llvm.org/D30286