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