[Lldb-commits] [PATCH] D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

[Greg Clayton via Phabricator via lldb-commits]

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, &sect_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, &sect_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, &sect_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, &sect_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