[Lldb-commits] [PATCH] D133038: Add SBDebugger::GetSetting()/GetSettings() public APIs

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/bindings/interface/SBDebugger.i:228
 
+    lldb::SBStructuredData GetSettings();
+
----------------
remove this and document the function below such that "setting" can be NULL or empty for to get all settings. Maybe also document that anything that you can type as an argument for "settings show" should work here. See the output of "settings show target" for an example.

Examples that should work here are:
```
lldb::SBStructuredData settings = debugger.GetSetting(nullptr); // Get all settings
lldb::SBStructuredData settings = debugger.GetSetting(""); // Get all settings
lldb::SBStructuredData settings = debugger.GetSetting("target.arg0"); // Get 1 specific setting
lldb::SBStructuredData settings = debugger.GetSetting("target.arg0 target.language"); // Get 2 specific settings
lldb::SBStructuredData settings = debugger.GetSetting("target"); // Get all target specific settings
```

So basically anything you can type into "settings show" should work here.



================
Comment at: lldb/include/lldb/API/SBDebugger.h:118
 
+  lldb::SBStructuredData GetSettings();
+
----------------
remove


================
Comment at: lldb/include/lldb/Interpreter/OptionValue.h:88
+  // child classes.
+  virtual llvm::json::Value ToJSON(const ExecutionContext *exe_ctx) {
+    return llvm::json::Value("<not yet implemented>");
----------------



================
Comment at: lldb/include/lldb/Interpreter/OptionValue.h:89
+  virtual llvm::json::Value ToJSON(const ExecutionContext *exe_ctx) {
+    return llvm::json::Value("<not yet implemented>");
+  }
----------------



================
Comment at: lldb/source/API/SBDebugger.cpp:439-455
+lldb::SBStructuredData SBDebugger::GetSettings() {
+  LLDB_INSTRUMENT_VA(this);
+
+  SBStructuredData data;
+  if (!m_opaque_sp)
+    return data;
+
----------------
Remove this function. We should have only one that takes a "const char *settings"


================
Comment at: lldb/source/API/SBDebugger.cpp:464
+
+  auto json_strm = std::make_shared<StreamString>();
+  ExecutionContext exe_ctx(
----------------
No need to create a shared pointer here.


================
Comment at: lldb/source/API/SBDebugger.cpp:467
+      m_opaque_sp->GetCommandInterpreter().GetExecutionContext());
+  m_opaque_sp->DumpPropertyValue(&exe_ctx, *json_strm, setting, /*dump_mask*/ 0,
+                                 /*is_json*/ true);
----------------



================
Comment at: lldb/source/API/SBDebugger.cpp:471
+  data.m_impl_up->SetObjectSP(
+      StructuredData::ParseJSON(json_strm->GetString().str()));
+  return data;
----------------



================
Comment at: lldb/source/Interpreter/OptionValueArray.cpp:81-83
+  for (uint32_t i = 0; i < size; ++i) {
+    json_array.emplace_back(m_values[i]->ToJSON(exe_ctx));
+  }
----------------
omit braces for single line for statement per llvm guidelines


================
Comment at: lldb/source/Interpreter/OptionValueDictionary.cpp:89-90
+  llvm::json::Array dict;
+  for (collection::iterator pos = m_values.begin(); pos != m_values.end();
+       ++pos) {
+    dict.emplace_back(llvm::json::Object{
----------------
Use modern C++ iterator style:

```
for (const auto &value: m_values) {
```


================
Comment at: lldb/source/Interpreter/OptionValueDictionary.cpp:92-93
+    dict.emplace_back(llvm::json::Object{
+        {"key", pos->first.GetCString()},
+        {"value", pos->second->ToJSON(exe_ctx)},
+    });
----------------
This is a dictionary, we already have a key/value to use


================
Comment at: lldb/test/API/commands/settings/TestSettings.py:806
+        settings_data.GetAsJSON(stream)
+        settings_json = json.loads(stream.GetData())
+        self.assertTrue(len(settings_json) > 0)
----------------
we should test values that have strings that contain reserved JSON characters. I believe '\' can be in a string and will need to be double desensitized. Also string values that contain the string quote characters like a string "hello \"world\""


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133038