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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Utility/Reproducer.h:59-64
+  void InitializeFileInfo(llvm::StringRef name,
+                          llvm::ArrayRef<std::string> files) {
+    m_info.name = name;
+    for (auto file : files)
+      m_info.files.push_back(file);
+  }
----------------
Could this be replaced by additional arguments to the constructor (avoiding two-stage initialization)?


================
Comment at: include/lldb/Utility/Reproducer.h:99
+  template <typename T> T *Get() {
+    auto it = m_providers.find(T::NAME);
+    if (it == m_providers.end())
----------------
Is the value of the `NAME` used anywhere? If not, you could just have each class define a `char` variable (like `llvm::ErrorInfo` subclasses do) and then use its address as a key, making everything faster.

(Even if name is useful (for printing to the user or whatever), it might be worth to switch to using the char-address for lookup and then just have `getName` method for other things.)


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:304
+    ProcessGDBRemoteProvider *provider =
+        g->GetOrCreate<ProcessGDBRemoteProvider>();
     // Set the history stream to the stream owned by the provider.
----------------
could `GetOrCreate` keep returning a reference (to avoid spurious `guaranteed to be not null` comments)?


================
Comment at: source/Utility/Reproducer.cpp:131-133
+  assert(m_providers.empty() &&
+         "Changing the root directory after providers have "
+         "been registered would invalidate the index.");
----------------
Given these limitations, would it be possible to set the root once in the constructor and keep it immutable since then?


================
Comment at: unittests/Utility/ReproducerTest.cpp:44-46
+    auto error = reproducer.SetReplay(FileSpec("/bogus/path"));
+    EXPECT_TRUE(static_cast<bool>(error));
+    llvm::consumeError(std::move(error));
----------------
Shorter and with better error messages:
`EXPECT_THAT_ERROR(reproducer.SetReplay(FileSpec("/bogus/path")), llvm::Succeeded());`


================
Comment at: unittests/Utility/ReproducerTest.cpp:79-81
+    auto error = reproducer.SetCapture(FileSpec("/bogus/path"));
+    EXPECT_TRUE(static_cast<bool>(error));
+    llvm::consumeError(std::move(error));
----------------
`EXPECT_THAT_ERROR(..., llvm::Failed());`


https://reviews.llvm.org/D54616