[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.

Stefan Gränitz via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri Nov 16 08:52:48 PST 2018

sgraenitz added a comment.

Looks pretty good to me, but added my 2¢ on minor things.

Comment at: include/lldb/Utility/Reproducer.h:169
+  llvm::Error SetCapture(bool value, FileSpec root = {});
+  llvm::Error SetReplay(bool value, FileSpec root = {});
Are the default-value-cases necessary?

Comment at: source/API/SBDebugger.cpp:1064
+        m_opaque_sp->SetReproducerReplay(llvm::StringRef::withNullAsEmpty(p));
+    llvm::consumeError(std::move(error));
+  }
Will the error be passed up the stack soon? Otherwise maybe add FIXME comment?

Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:303
+    if (ProcessGDBRemoteProvider *provider =
+            g->GetOrCreate<ProcessGDBRemoteProvider>()) {
+      // Set the history stream to the stream owned by the provider.
In which case would `GetOrCreate()` fail?

Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3440
   // Construct replay history path.
-  FileSpec history_file(loader->GetDirectory());
+  FileSpec history_file = (loader->GetRoot());

Comment at: unittests/Utility/ReproducerTest.cpp:37
+    auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
+    EXPECT_FALSE(static_cast<bool>(error));
+    EXPECT_NE(nullptr, reproducer.GetGenerator());
It may be good to know what went wrong if this fails. And: are the other `EXPECT`s still relevant in the error-case?

If `SetCapture()` can fail in this particular situation, you may want to do something like:
if (error)
  FAIL() << llvm::toString(std::move(error));
Otherwise, if this was only to prepare the test and you wanted to indicate that it's not supposed to fail, then you could do:
llvm::cantFail(reproducer.SetCapture(true, FileSpec("/bogus/path"));



More information about the lldb-commits mailing list