-
Notifications
You must be signed in to change notification settings - Fork 16k
[lldb][Expression] Add API to set/get language-specific expression options #179208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4adc40f
f1d8205
e468faa
a84c451
2deb903
e1591e3
1a0276b
dc3a7ef
6a3032a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,6 +70,7 @@ | |
|
|
||
| #include "llvm/ADT/ScopeExit.h" | ||
| #include "llvm/ADT/SetVector.h" | ||
| #include "llvm/Support/ErrorExtras.h" | ||
| #include "llvm/Support/ThreadPool.h" | ||
|
|
||
| #include <memory> | ||
|
|
@@ -5357,3 +5358,43 @@ void Target::NotifyBreakpointChanged( | |
| if (EventTypeHasListeners(Target::eBroadcastBitBreakpointChanged)) | ||
| BroadcastEvent(Target::eBroadcastBitBreakpointChanged, breakpoint_data_sp); | ||
| } | ||
|
|
||
| llvm::Error | ||
| EvaluateExpressionOptions::SetBooleanLanguageOption(llvm::StringRef option_name, | ||
| bool value) { | ||
| if (option_name.empty()) | ||
| return llvm::createStringError("Can't set an option with an empty name."); | ||
|
|
||
| GetLanguageOptions().AddBooleanItem(option_name, value); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There doesn't seem to be any system yet for the Language to validate the options you are setting in it. It would be a stretch to require you to put that machinery in place just for this option. But we should at least have a big FIXME here because we really should come up with a way to do that. And we should probably add a note to the documentation that at present we don't validate the spelling of options set this way.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Other than that, this LGTM. |
||
|
|
||
| return llvm::Error::success(); | ||
| } | ||
|
|
||
| llvm::Expected<bool> EvaluateExpressionOptions::GetBooleanLanguageOption( | ||
| llvm::StringRef option_name) const { | ||
| const StructuredData::Dictionary &opts = GetLanguageOptions(); | ||
|
|
||
| if (!opts.HasKey(option_name)) | ||
| return llvm::createStringErrorV("Option '{0}' does not exist.", | ||
| option_name); | ||
|
|
||
| bool result; | ||
| if (!opts.GetValueForKeyAsBoolean(option_name, result)) | ||
| return llvm::createStringErrorV("Failed to get option '{0}' as boolean.", | ||
| option_name); | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| const StructuredData::Dictionary & | ||
| EvaluateExpressionOptions::GetLanguageOptions() const { | ||
| assert(m_language_options_sp); | ||
|
|
||
| return *m_language_options_sp; | ||
| } | ||
|
|
||
| StructuredData::Dictionary &EvaluateExpressionOptions::GetLanguageOptions() { | ||
| assert(m_language_options_sp); | ||
|
|
||
| return *m_language_options_sp; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's weird to a getter specifically for boolean options but have the setter name be "generic" but take a boolean argument. I also think this is going to break in Python if you add overloads for the setter with different argument types like void
SetLanguageOption(const char *option_name, int value).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my assumption was that we can overload functions. Pretty sure SWIG is capable of it? But do we avoid doing overloads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can overload functions. SWIG will handle it correctly with
argsorkwargsin python.I think Ismail's point is one of consistency. The getter has
Booleanin the name but the setter doesn't (even though the two methods are supposed to be matching). I think changing the setter name would resolve this.To go further, does
GetLanguageOptionAsBooleanmean you can get a non-boolean option value as a boolean? Maybe something likeGetBooleanLanguageOptionis clearer? To make it matchy-matchy you'd also wantSetBooleanLanguageOption.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll rename the two as suggested!