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

[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/include/lldb/API/SBDebugger.h:119
+  /// Get all settings into SBStructuredData.
+  lldb::SBStructuredData GetSetting();
+
----------------
We still don't need two actual APIs in SBDebugger.h. Remove this.


================
Comment at: lldb/include/lldb/API/SBDebugger.h:136
+  ///
+  lldb::SBStructuredData GetSetting(const char *setting);
+
----------------
We can default "settings = nullptr" here so users can call without anything, but we should have only 1 API function for getting the settings.


================
Comment at: lldb/include/lldb/Interpreter/OptionValue.h:89
+  virtual llvm::json::Value ToJSON(const ExecutionContext *exe_ctx) {
+    return nullptr;
+  }
----------------
Add a comment here. Suggested comment added.


================
Comment at: lldb/include/lldb/Interpreter/OptionValueChar.h:34-37
+    if (m_current_value != '\0')
+      return m_current_value;
+    else
+      return "(null)";
----------------
Since this is actually a character, it should store what ever the character value is. So if the character is zero, it should store zero, not a string "(null)"


================
Comment at: lldb/source/API/SBDebugger.cpp:439-443
+lldb::SBStructuredData SBDebugger::GetSetting() {
+  LLDB_INSTRUMENT_VA(this);
+  return GetSetting(nullptr);
+}
+
----------------
Remove this and only keep the one that takes a 'const char *setting"'


================
Comment at: lldb/source/Interpreter/OptionValueDictionary.cpp:92
+  }
+  return std::move(dict);
+}
----------------
I think this move can be removed? Watch for warnings here. Some buildbots have warnings as errors which could cause the submission to be reverted.


================
Comment at: lldb/test/API/commands/settings/TestSettings.py:799
+        arg_value = "hello \"world\""
+        self.runCmd('settings set target.arg0 %s' % arg_value)
+
----------------
We need to set a few more settings for anything we support the ToJSON for to ensure they function as expected.


================
Comment at: lldb/test/API/functionalities/source-map/TestTargetSourceMap.py:25
+        src_dir = self.getSourceDir()
+        self.runCmd('settings set target.source-map . "%s"' % src_dir)
+
----------------
hawkinsw wrote:
> yinghuitan wrote:
> > hawkinsw wrote:
> > > Sorry if this comment is not helpful, but I was wondering ...
> > > 
> > > Could we use `source_map_setting_path` here? That would make future changes easier?
> > @hawkinsw, sorry, almost missed your comment. There is no single value for source map path but a pair of original/replacement. In this case, original is "." while the replacement is "src_dir" so I think src_dir makes sense here. 
> No problem! I am sorry I miscommunicated! I just meant, could we write something like
> 
> ```
> self.runCmd('settings set %s . "%s"' % (source_map_setting_path, src_dir))
> ```
Good idea!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133038