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