labath added a comment. Looks great, just a couple of style nits in the tests.
================ Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:82 +#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringSwitch.h" ---------------- Is this include still necessary? ================ Comment at: unittests/Signals/UnixSignalsTest.cpp:43 + +#define ASSERT_EQ_ARRAYS(expected, observed) \ + AssertEqArrays((expected), (observed), __FILE__, __LINE__); ---------------- This (and the function) should probably have an EXPECT_.. prefix instead, as it does not abort the evaluation of the function it is in (like other ASSERT_*** macros). ================ 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); ---------------- We generally put the "expected" value first, and the observed second. The ASSERT_ macro does that, but here you seem to have inverted it. https://reviews.llvm.org/D30520 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
