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