[Lldb-commits] [PATCH] D133038: Add SBDebugger::GetSetting() public API

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/Core/UserSettingsController.h:62-64
   virtual void DumpAllPropertyValues(const ExecutionContext *exe_ctx,
-                                     Stream &strm, uint32_t dump_mask);
+                                     Stream &strm, uint32_t dump_mask,
+                                     bool is_json = false);
----------------
A flag doesn't scale if we ever decide we need another format. An enum would solve that and would eliminate the need for a comment `/*is_json=*/. I think the two values could be `Plain` (or `Text`) and `JSON`. 


================
Comment at: lldb/include/lldb/Interpreter/OptionValue.h:86-87
 
+  // TODO: make this function pure virtual after implementing it in all
+  // child classes.
+  virtual llvm::json::Value ToJSON(const ExecutionContext *exe_ctx) {
----------------
Is this done?


================
Comment at: lldb/source/Core/UserSettingsController.cpp:63
+  if (is_json) {
+    llvm::json::Value json = properties_sp->ToJSON(exe_ctx);
+    strm.Printf("%s", llvm::formatv("{0:2}", json).str().c_str());
----------------
Earlier you said that `ToJSON` should never return NULL. We should enforce that with an assert here.


================
Comment at: lldb/source/Interpreter/OptionValueArray.cpp:80-81
+  llvm::json::Array json_array;
+  const uint32_t size = m_values.size();
+  for (uint32_t i = 0; i < size; ++i)
+    json_array.emplace_back(m_values[i]->ToJSON(exe_ctx));
----------------



================
Comment at: lldb/source/Interpreter/OptionValueDictionary.cpp:89-91
+  for (const auto &value : m_values) {
+    dict.try_emplace(value.first.GetCString(), value.second->ToJSON(exe_ctx));
+  }
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133038/new/

https://reviews.llvm.org/D133038