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