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

[Stefan Gränitz via Phabricator via lldb-commits]

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());
   history_file.AppendPathComponent(provider_info->files.front());
----------------
Brackets


================
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"));
```


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54616