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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
