clayborg added inline comments.
================
Comment at: lldb/include/lldb/Core/Debugger.h:594
lldb::TargetSP m_dummy_target_sp;
+ Diagnostics::CallbackID m_diagnostics_callback_id;
----------------
I am guessing these changes are in Debugger.h and Debugger.cpp are not related
to this diff?
================
Comment at: lldb/source/Core/Debugger.cpp:830-845
+ if (Diagnostics::Enabled()) {
+ m_diagnostics_callback_id = Diagnostics::Instance().AddCallback(
+ [this](const FileSpec &dir) -> llvm::Error {
+ for (auto &entry : m_stream_handlers) {
+ llvm::StringRef log_path = entry.first();
+ llvm::StringRef file_name = llvm::sys::path::filename(log_path);
+ FileSpec destination = dir.CopyByAppendingPathComponent(file_name);
----------------
I am guessing these changes are in Debugger.h and Debugger.cpp are not related
to this diff?
================
Comment at: lldb/source/Symbol/Symbol.cpp:780-781
+
+bool fromJSON(const llvm::json::Value &value, lldb_private::JSONSymbol &symbol,
+ llvm::json::Path path) {
+ llvm::json::ObjectMapper o(value, path);
----------------
Should this return an llvm::Expected<lldb_private::JSONSymbol> instead of a
bool? Or is this fromJSON pattern used everywhere? Then we wouldn't need to
fill in "path" and could return an error?
================
Comment at: lldb/source/Symbol/Symbol.cpp:804-805
+
+bool fromJSON(const llvm::json::Value &value, lldb::SymbolType &type,
+ llvm::json::Path path) {
+ if (auto str = value.getAsString()) {
----------------
Should this return an llvm::Expected<SymbolType> instead of a bool? Or is this
fromJSON pattern used everywhere? Then we wouldn't need to fill in "path" and
could return an error?
================
Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:34
+ Expected<json::Value> json = json::parse(text);
+ ASSERT_TRUE(static_cast<bool>(json));
+
----------------
Change over to using EXPECT_THAT_EXPECTED. Repeat for all cases below.
================
Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:56
+ Expected<Symbol> symbol = Symbol::FromJSON(json_symbol, §_list);
+ ASSERT_TRUE(static_cast<bool>(symbol));
+ EXPECT_EQ(symbol->GetName(), ConstString("foo"));
----------------
Use EXPECT_THAT_EXPECTED for clarity
================
Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:81
+ Expected<Symbol> symbol = Symbol::FromJSON(json_symbol, §_list);
+ ASSERT_TRUE(static_cast<bool>(symbol));
+ EXPECT_EQ(symbol->GetName(), ConstString("foo"));
----------------
================
Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:143-144
+ Expected<Symbol> symbol = Symbol::FromJSON(json_symbol, nullptr);
+ ASSERT_FALSE(static_cast<bool>(symbol));
+ EXPECT_EQ(toString(symbol.takeError()), g_error_no_section_list);
+}
----------------
Use EXPECT_THAT_EXPECTED for all "Expected<Symbol>" error cases.
================
Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:155-156
+ Expected<Symbol> symbol = Symbol::FromJSON(json_symbol, §_list);
+ ASSERT_FALSE(static_cast<bool>(symbol));
+ EXPECT_EQ(toString(symbol.takeError()), g_error_both_value_and_address);
+}
----------------
Use EXPECT_THAT_EXPECTED for all "Expected<Symbol>" error cases.
================
Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:165-166
+ Expected<Symbol> symbol = Symbol::FromJSON(json_symbol, §_list);
+ ASSERT_FALSE(static_cast<bool>(symbol));
+ EXPECT_EQ(toString(symbol.takeError()), g_error_neither_value_or_address);
+}
----------------
Use EXPECT_THAT_EXPECTED for all "Expected<Symbol>" error cases.
================
Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:189-191
+ ASSERT_FALSE(static_cast<bool>(symbol));
+ EXPECT_EQ(toString(symbol.takeError()),
+ "no section found for address: 0xfff");
----------------
Use EXPECT_THAT_EXPECTED for all "Expected<Symbol>" error cases
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145180/new/
https://reviews.llvm.org/D145180
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
