labath added a comment.

In general, I'd prefer to avoid adding new methods to the class under test, and 
save subclassing for the cases where it's impossible to do things otherwise 
(abstract class, visibility restrictions (although that often means you're not 
testing what you should be)). These things could live inside an gtest fixture 
(subclass of testing::Test), which could have a DynamicRegisterInfo member if 
needed.



================
Comment at: lldb/unittests/Process/Utility/DynamicRegisterInfoTest.cpp:51-52
+                          std::vector<uint32_t> invalidate_regs = {}) {
+    std::string reg_msg =
+        llvm::formatv("at register {0} (num: {1})", reg_name, reg_num);
+    const RegisterInfo *reg = GetRegisterInfoAtIndex(reg_num);
----------------
Maybe use `SCOPED_TRACE` instead?


================
Comment at: lldb/unittests/Process/Utility/DynamicRegisterInfoTest.cpp:61-93
+    if (value_regs.empty())
+      EXPECT_EQ(reg->value_regs, nullptr) << reg_msg;
+    else {
+      EXPECT_NE(reg->value_regs, nullptr) << reg_msg;
+
+      size_t i;
+      for (i = 0; i < value_regs.size(); i++) {
----------------
I'd probably write a helper function to convert the register lists to a vector, 
and then have gtest compare the vectors:
```
std::vector<uint32_t> to_vector(uint32_t *regs) {
  std::vector<uint32_t> result;
  if (regs) {
    while(*regs != LLDB_INVALID_REGNUM)
      result.push_back(*regs++);
  }
  return result;
}
...
ASSERT_THAT(to_vector(info->value_regs), value_regs);
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109906/new/

https://reviews.llvm.org/D109906

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to