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

[Pavel Labath via Phabricator via lldb-commits]

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