[Lldb-commits] [PATCH] D30520: Make LLDB skip server-client roundtrip for signals that don't require any actions

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Pretty close. My only objection is we have many "lldb_private::Process::Will" and "lldb_private::Process::Did" prefixed functions and none of them are required to call the superclass version. I would prefer that this doesn't change. See my inlined comments,



================
Comment at: source/Target/Process.cpp:1624-1627
+Error Process::WillResume() {
+  UpdateAutomaticSignalFiltering();
+  return Error();
+}
----------------
I would prefer to not require people to call the base class functions for any "Process::Will*" or "Process::Did*" there are many of these and the ideas are that these are things that can be overridden. These aren't called in that many places, so it will be easy to just call this function before calling it, or adding a Process::PrivateWillResume that calls UpdateAutomaticSignalFiltering() followed by Process::WillResume().


================
Comment at: source/Target/Process.cpp:1629
+
+void Process::DidLaunch() { UpdateAutomaticSignalFiltering(); }
+
----------------
Ditto.


================
Comment at: unittests/Signals/UnixSignalsTest.cpp:64
+      signals.GetSignalInfo(signo, should_suppress, should_stop, should_notify);
+  EXPECT_EQ(name, "SIG4");
+  EXPECT_EQ(should_suppress, true);
----------------
labath wrote:
> We generally put the "expected" value first, and the observed second. The ASSERT_ macro does that, but here you seem to have inverted it.
Is this "expected" value being first documented somewhere? If not it should be. I have seen this comment a few times and it would be great if the headerdoc already says this. 


https://reviews.llvm.org/D30520