[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you. I like the new test, but see my comment about avoiding duplicating macros in the test. Besides returning objects, I can think of a couple of more interesting scenarios to cover:

- recursive calls (to exercise the g_global_boundary thingy)
- having two objects with identical `this` pointers (you can use placement `new` to guarantee that)
- calling the "custom" function during replay



================
Comment at: unittests/Utility/ReproducerInstrumentationTest.cpp:60-62
+template <typename T> bool Equals(T LHS, T RHS) {
+  return std::fabs(RHS - LHS) < std::numeric_limits<T>::epsilon();
+}
----------------
googletest already has apis for this: EXPECT_FLOAT/DOUBLE_EQ or EXPECT_NEAR.


================
Comment at: unittests/Utility/ReproducerInstrumentationTest.cpp:67-122
+#define SB_RECORD_CONSTRUCTOR(Class, Signature, ...)                           \
+  lldb_private::repro::Recorder sb_recorder(g_serializer, g_registry,          \
+                                            LLVM_PRETTY_FUNCTION);             \
+  sb_recorder.Record(&lldb_private::repro::construct<Class Signature>::doit,   \
+                     __VA_ARGS__);                                             \
+  sb_recorder.RecordResult(this);
+
----------------
Since these macros are the actual "public" interface of the instrumentation framework, and they do contain non-trivial logic, I think it would be nice to test the real macros, instead of redefining them here. (It would also be nice to simply avoid duplicating code.)

Would something like this work:
- move the macros to ReproducerInstrumentation.h (maybe also rename to `LLDB_RECORD_blah_blah`).
- parameterize them on a macro for getting the instrumentation classes. So, something like:
```

#define LLDB_RECORD_CONSTRUCTOR(Class, Signature, ...)                           \
  if (InstrumentationData data = LLDB_GET_INSTRUMENTATION_DATA()) {  \
    lldb_private::repro::Recorder recorder(                                 \
        data.GetSerializer(),                   \
        data.GetRegistry(),                     \
        LLVM_PRETTY_FUNCTION);                                                 \
    sb_recorder.Record(&lldb_private::repro::construct<Class Signature>::doit, \
                       __VA_ARGS__);                                           \
    sb_recorder.RecordResult(this);                                            \
  }
```
where `InstrumentationData` is essentially `std::pair<Serializer *, Registry *>` with a suitable `operator bool`. Then the tests could define `LLDB_GET_INSTRUMENTATION_DATA` simply to `InstrumentationData(&g_serializer, &g_registry)` and the "real" code could do something like
```
#define LLDB_GET_INSTRUMENTATION_DATA lldb_private::repro::GetInstrumentationData()

InstrumentationData GetInstrumentationData() {
  auto *g = lldb_private::repro::Reproducer::Instance().GetGenerator();
  if (!g) return {};
  auto &p = g->GetOrCreate<SBProvider>();
  return {p.GetSerializer(), p.GetRegistry();
}
```


================
Comment at: unittests/Utility/ReproducerInstrumentationTest.cpp:143-148
+    EXPECT_EQ(g_a, 0);
+    EXPECT_EQ(g_b, 0);
+    EXPECT_EQ(g_c, 0);
+    EXPECT_EQ(g_d, "");
+    EXPECT_EQ(g_e, 0);
+    EXPECT_EQ(g_f, false);
----------------
This seems rather pointless, as you've just set the variables to those values. What are you testing here? The compiler?


================
Comment at: unittests/Utility/ReproducerInstrumentationTest.cpp:384
+  llvm::raw_string_ostream os(str);
+  g_serializer = Serializer(os);
+
----------------
Make g_serializer `llvm:Optional`, and then you can avoid needing it be copyable (I think making the Serializer class copyable is a bad idea for the same reason than making raw_ostream copyable would be a bad idea)


================
Comment at: unittests/Utility/ReproducerInstrumentationTest.cpp:399-404
+  EXPECT_EQ(foo.g_a, 100);
+  EXPECT_EQ(foo.g_b, 200);
+  EXPECT_TRUE(Equals(foo.g_c, c));
+  EXPECT_EQ(foo.g_d, "bar");
+  EXPECT_TRUE(Equals(foo.g_e, e));
+  EXPECT_EQ(foo.g_f, true);
----------------
access variables via the class (InstrumentedFoo::g_a) to make it clear that they are static.


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

https://reviews.llvm.org/D56322