labath added a comment.
I think we're very close. This batch of comments is just about small
improvements to the tests. It looks like the `Validate` thingy wasn't such a
clever idea, since the fact that the framework creates extra copies means that
there are some unvalidated copies floating around. Still, I think it's worth it
overall, because it tests the bahavior we want more closely than if we just
went through global variables (however, do see my comments on the global
variables to make sure that validation did indeed take place).
================
Comment at: include/lldb/Utility/ReproducerInstrumentation.h:594-595
+
+ /// The serializer is set from the reproducer framework. If the serializer is
+ /// not set, we're not in recording mode.
+ Serializer &m_serializer;
----------------
I guess this comment is no longer true. We should only ever create this object
if we are recording.
================
Comment at: source/API/SBReproducerPrivate.h:23
+
+#define LLDB_GET_INSTRUMENTATION_DATA
\
+ lldb_private::repro::GetInstrumentationData()
----------------
If it's all the same to you, i'd prefer to have this be a function-like macro
(so just add `()` after the macro name here, and also add `()` to all macro
"invocations").
================
Comment at: source/API/SBReproducerPrivate.h:61
+
+InstrumentationData GetInstrumentationData() {
+ if (auto *g = lldb_private::repro::Reproducer::Instance().GetGenerator()) {
----------------
`inline` (or move to .cpp)
================
Comment at: unittests/Utility/ReproducerInstrumentationTest.cpp:50-52
+template <typename T> bool Equals(T LHS, T RHS) {
+ return std::fabs(RHS - LHS) < std::numeric_limits<T>::epsilon();
+}
----------------
I guess this is no longer used?
================
Comment at: unittests/Utility/ReproducerInstrumentationTest.cpp:62
+public:
+ ~InstrumentedFoo() { EXPECT_TRUE(m_validated); }
+
----------------
I think it's still important to independently check that `Validate` was called,
IIUC, the framework never deletes the objects it creates, so this wouldn't
actually check anything during the replay. Even if it did, this opens up the
possibility that the `Replay` method decides to do absolutely nothing for some
reason, in which case the test would still succeed. The idea for stashing the
`this` pointer from the other comment could be reused for this purpose too.
================
Comment at: unittests/Utility/ReproducerInstrumentationTest.cpp:443
+
+ InstrumentedFoo *foo2 = new (foo) InstrumentedFoo();
+ foo2->A(100);
----------------
destroy the old object before creating a new one in its place
(`foo->~InstrumentedFoo()`).
================
Comment at: unittests/Utility/ReproducerInstrumentationTest.cpp:455
+ registry.Replay(os.str());
+}
+
----------------
A good thing to check here would be to make sure that the framework created
actually created two instances of this object. As it stands now, if the
framework just decided to keep calling methods on the original object, the test
would probably still succeed. One way to achieve that would be to enhance the
Validate method to stash the `this` pointer into an array somewhere. Then you
could check that the array contains two elements (and that they are different).
================
Comment at: unittests/Utility/ReproducerInstrumentationTest.cpp:479-480
+
+ bar.SetInstrumentedFoo(foo);
+ bar.SetInstrumentedFoo(&foo);
+ bar.Validate();
----------------
Add a check that these two methods are called with the same object during
replay. The methods could store the object pointer instead of just a plain
flag, and then Validate could verify their equality.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56322/new/
https://reviews.llvm.org/D56322
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
