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