[lldb][NFCI] Remove CommandObjectProcessHandle::VerifyCommandOptionValue #79901
Conversation
|
@llvm/pr-subscribers-lldb Author: Alex Langford (bulbazord) ChangesI was refactoring something else but ran into this function. It was somewhat confusing to read through and understand, but it boils down to two steps:
Instead of having an integer out param and a bool return value, the interface is better served with an optional<bool> -- Either it parses into true or false, or you get back nothing (nullopt). Full diff: https://github.com/llvm/llvm-project/pull/79901.diff 1 Files Affected:
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index c7b874d1979377..f089a86275dc69 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -1591,24 +1591,24 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
Options *GetOptions() override { return &m_options; }
- bool VerifyCommandOptionValue(const std::string &option, int &real_value) {
- bool okay = true;
+ std::optional<bool> VerifyCommandOptionValue(const std::string &option) {
+ if (option.empty())
+ return std::nullopt;
+
bool success = false;
bool tmp_value = OptionArgParser::ToBoolean(option, false, &success);
+ if (success)
+ return tmp_value;
+
+ int parsed_value = -1;
+ if (llvm::to_integer(option, parsed_value)) {
+ if (parsed_value != 0 && parsed_value != 1)
+ return std::nullopt;
- if (success && tmp_value)
- real_value = 1;
- else if (success && !tmp_value)
- real_value = 0;
- else {
- // If the value isn't 'true' or 'false', it had better be 0 or 1.
- if (!llvm::to_integer(option, real_value))
- real_value = 3;
- if (real_value != 0 && real_value != 1)
- okay = false;
+ return parsed_value == 0 ? false : true;
}
- return okay;
+ return std::nullopt;
}
void PrintSignalHeader(Stream &str) {
@@ -1666,33 +1666,31 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
// the user's options.
ProcessSP process_sp = target.GetProcessSP();
- int stop_action = -1; // -1 means leave the current setting alone
- int pass_action = -1; // -1 means leave the current setting alone
- int notify_action = -1; // -1 means leave the current setting alone
+ std::optional<bool> stop_action = VerifyCommandOptionValue(m_options.stop);
+ std::optional<bool> pass_action = VerifyCommandOptionValue(m_options.pass);
+ std::optional<bool> notify_action =
+ VerifyCommandOptionValue(m_options.notify);
- if (!m_options.stop.empty() &&
- !VerifyCommandOptionValue(m_options.stop, stop_action)) {
+ if (!m_options.stop.empty() && !stop_action.has_value()) {
result.AppendError("Invalid argument for command option --stop; must be "
"true or false.\n");
return;
}
- if (!m_options.notify.empty() &&
- !VerifyCommandOptionValue(m_options.notify, notify_action)) {
- result.AppendError("Invalid argument for command option --notify; must "
- "be true or false.\n");
+ if (!m_options.pass.empty() && !pass_action.has_value()) {
+ result.AppendError("Invalid argument for command option --pass; must be "
+ "true or false.\n");
return;
}
- if (!m_options.pass.empty() &&
- !VerifyCommandOptionValue(m_options.pass, pass_action)) {
- result.AppendError("Invalid argument for command option --pass; must be "
- "true or false.\n");
+ if (!m_options.notify.empty() && !notify_action.has_value()) {
+ result.AppendError("Invalid argument for command option --notify; must "
+ "be true or false.\n");
return;
}
- bool no_actions = (stop_action == -1 && pass_action == -1
- && notify_action == -1);
+ bool no_actions = (!stop_action.has_value() && !pass_action.has_value() &&
+ !notify_action.has_value());
if (m_options.only_target_values && !no_actions) {
result.AppendError("-t is for reporting, not setting, target values.");
return;
@@ -1732,14 +1730,14 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
if (signo != LLDB_INVALID_SIGNAL_NUMBER) {
// Casting the actions as bools here should be okay, because
// VerifyCommandOptionValue guarantees the value is either 0 or 1.
- if (stop_action != -1)
- signals_sp->SetShouldStop(signo, stop_action);
- if (pass_action != -1) {
- bool suppress = !pass_action;
+ if (stop_action.has_value())
+ signals_sp->SetShouldStop(signo, *stop_action);
+ if (pass_action.has_value()) {
+ bool suppress = !*pass_action;
signals_sp->SetShouldSuppress(signo, suppress);
}
- if (notify_action != -1)
- signals_sp->SetShouldNotify(signo, notify_action);
+ if (notify_action.has_value())
+ signals_sp->SetShouldNotify(signo, *notify_action);
++num_signals_set;
} else {
result.AppendErrorWithFormat("Invalid signal name '%s'\n",
@@ -1759,40 +1757,35 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
}
num_signals_set = num_args;
}
- auto set_lazy_bool = [] (int action) -> LazyBool {
- LazyBool lazy;
- if (action == -1)
- lazy = eLazyBoolCalculate;
- else if (action)
- lazy = eLazyBoolYes;
- else
- lazy = eLazyBoolNo;
- return lazy;
+ auto set_lazy_bool = [](std::optional<bool> action) -> LazyBool {
+ if (!action.has_value())
+ return eLazyBoolCalculate;
+ return (*action) ? eLazyBoolYes : eLazyBoolNo;
};
// If there were no actions, we're just listing, don't add the dummy:
if (!no_actions)
- target.AddDummySignal(arg.ref(),
- set_lazy_bool(pass_action),
- set_lazy_bool(notify_action),
- set_lazy_bool(stop_action));
+ target.AddDummySignal(arg.ref(), set_lazy_bool(pass_action),
+ set_lazy_bool(notify_action),
+ set_lazy_bool(stop_action));
}
} else {
// No signal specified, if any command options were specified, update ALL
// signals. But we can't do this without a process since we don't know
// all the possible signals that might be valid for this target.
- if (((notify_action != -1) || (stop_action != -1) || (pass_action != -1))
- && process_sp) {
+ if ((notify_action.has_value() || stop_action.has_value() ||
+ pass_action.has_value()) &&
+ process_sp) {
if (m_interpreter.Confirm(
"Do you really want to update all the signals?", false)) {
int32_t signo = signals_sp->GetFirstSignalNumber();
while (signo != LLDB_INVALID_SIGNAL_NUMBER) {
- if (notify_action != -1)
- signals_sp->SetShouldNotify(signo, notify_action);
- if (stop_action != -1)
- signals_sp->SetShouldStop(signo, stop_action);
- if (pass_action != -1) {
- bool suppress = !pass_action;
+ if (notify_action.has_value())
+ signals_sp->SetShouldNotify(signo, *notify_action);
+ if (stop_action.has_value())
+ signals_sp->SetShouldStop(signo, *stop_action);
+ if (pass_action.has_value()) {
+ bool suppress = !*pass_action;
signals_sp->SetShouldSuppress(signo, suppress);
}
signo = signals_sp->GetNextSignalNumber(signo);
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
This version is still a little awkward because there's an ambiguity about what the incoming string being empty means. In the VerifyCommandOptionValue, it means "No Value" and the return is not distinguishable from "error" - but in a bunch of the places where you use VerifyCommandOptionValue an empty string just means a value wasn't provided, so you end up with constructs where you set the optional (checking the incoming string for I wonder if it would be better to add a Status to the call. Then empty input -> "Empty Optional but no error"; error in parsing -> "Empty Optional but error" and result means success. Then you wouldn't have to do two checks on the input string in different places, or make up the error string. |
I did this intentionally so that
|
By treating "empty string" exactly the same as "parse error" this kind of is interpreting the results, requiring a "pre-interpretation" to shadow this. But the only thing VerifyCommandOptionValue adds in this case is treating 0 => false and 1 => true, and it's not at all clear to me why that isn't the job of ToBoolean. And that API is weird anyway, it takes an input string, checks whether it can reasonably be interpreted as a boolean (including 0->false, 1->true) then converts the boolean value back to an integer value of 0 or 1. It's very possible this is my doing, I can't remember, but that is kind of odd to begin with. And why is it only used for these three booleans and no others? I think getting rid of it and just using ToBoolean seems the better path.
|
There was a problem hiding this comment.
We should be modify OptionArgParser::ToBoolean(...) to return a std::optional<bool> instead of a local function. It already handles integer strings "0" and "1" correctly. Granted it doesn't allow "00" or "01" to be specified, but I am not sure we care. If we do, we can fix OptionArgParser::ToBoolean(...)
| @@ -1591,24 +1591,24 @@ class CommandObjectProcessHandle : public CommandObjectParsed { | |||
|
|
|||
| Options *GetOptions() override { return &m_options; } | |||
|
|
|||
| bool VerifyCommandOptionValue(const std::string &option, int &real_value) { | |||
| bool okay = true; | |||
| std::optional<bool> VerifyCommandOptionValue(const std::string &option) { | |||
There was a problem hiding this comment.
We should modify OptionArgParser::ToBoolean(...) to return a std::optional<bool>. it already handles "0" and "1" strings correctly, so this integer conversion stuff is just not doing anything useful
There was a problem hiding this comment.
I was actually in the middle of doing that when I found VerifyCommandOptionValue. I was going to upload a PR for it after removing this.
|
Looks like I wasn't the one that added this, so I don't feel so bad I can't remember why I did it this way...
I think this VerifyCommandOptionValue is just a historical artifact, we should remove it and do what all the other boolean options do.
Jim
… On Jan 29, 2024, at 4:50 PM, Greg Clayton ***@***.***> wrote:
@clayborg requested changes on this pull request.
We should be modify OptionArgParser::ToBoolean(...) to return a std::optional<bool> instead of a local function. It already handles integer strings "0" and "1" correctly. Granted it doesn't allow "00" or "01" to be specified, but I am not sure we care. If we do, we can fix OptionArgParser::ToBoolean(...)
In lldb/source/Commands/CommandObjectProcess.cpp <#79901 (comment)>:
> @@ -1591,24 +1591,24 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
Options *GetOptions() override { return &m_options; }
- bool VerifyCommandOptionValue(const std::string &option, int &real_value) {
- bool okay = true;
+ std::optional<bool> VerifyCommandOptionValue(const std::string &option) {
We should modify OptionArgParser::ToBoolean(...) to return a std::optional<bool>. it already handles "0" and "1" strings correctly, so this integer conversion stuff is just not doing anything useful
—
Reply to this email directly, view it on GitHub <#79901 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW6RAW4LLT7WOIX4GGLYRA7UVAVCNFSM6AAAAABCQEQH7OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNJQGAZTENRXGE>.
You are receiving this because your review was requested.
|
|
Okay, here's what I'll do then:
|
…andOptionValue I was refactoring something else but ran into this function. It was somewhat confusing to read through and understand, but it boils down to two steps: - First we try `OptionArgParser::ToBoolean`. If that works, then we're good to go. - Second, we try `llvm::to_integer` to see if it's an integer. If it parses to 0 or 1, we're good. - Failing either of the steps above means we cannot parse it into a bool. Instead of having an integer out param and a bool return value, it seems like the interface is better served with an optional<bool> -- Either it parses into true or false, or you get back nothing (nullopt).
bulbazord
force-pushed
the
verify-command-option-value
branch
from
3a7810a to
8e78455
Compare
|
Sounds like a good plan. |
bulbazord
changed the title
| bool success = false; | ||
| bool value = OptionArgParser::ToBoolean(m_options.stop, false, &success); | ||
| if (!success) { | ||
| result.AppendError( |
There was a problem hiding this comment.
I don't think this is up to you, since the old code also lied. But this error is not true, the values can also be yes, no, 1, 0. Really should be: should be a <boolean> value, and then all we have to do is teach help <boolean> not to lie:
(lldb) help <boolean> <boolean> -- A Boolean value: 'true' or 'false'
It might be nice if these OptionArgParser::ToWhatever functions could help produce the error string, it's awkward that everybody has to make up their own version of the "what is right" string.
|
@clayborg how does this look to you? |
|
ping @clayborg How does this look now? |
| if (!m_options.stop.empty()) { | ||
| bool success = false; | ||
| bool value = OptionArgParser::ToBoolean(m_options.stop, false, &success); |
There was a problem hiding this comment.
Why don't we do this error checking in CommandObjectProcessHandle::CommandOptions::SetOptionValue(...)? We can return an error from there instead of making it successfully though option parsing only to look closer at the option values and return an error later in this function?
Also, I see many places use this version of OptionArgParser::ToBoolean(...), and there is a ton of duplicated code like:
- call
OptionArgParser::ToBoolean(...) - check value of success
- make error string and everyone picks their own "hey this isn't a valid boolean value (like Jim already mentioned below).
Since there are already so many uses of the current and only version of OptionArgParser::ToBoolean(...), maybe we make an overload of this:
llvm::Expected<bool> OptionArgParser::ToBoolean(llvm::StringRef ref);
Then in the `CommandObjectProcessHandle::CommandOptions::SetOptionValue(...) function we can do:
case 's':
if (auto ExpectedBool = OptionArgParser::ToBoolean(option_arg)) {
stop = *ExpectedBool;
else
error = Status(ExpectedBool.takeError());
And the new OptionArgParser::ToBoolean() overload would return an error as Jim wants below:
"Invalid <boolean> value "%s"
Where %s gets substitued with "option_arg.str().c_str()". And then the help would help people figure this out.
There was a problem hiding this comment.
My plan (that I wrote up 2 weeks ago) was to remove VerifyCommandOptionValue entirely in this commit and then refactor ToBoolean the way you suggested as a follow-up to this change. Does that sound good to you?
I was refactoring something else but ran into this function. It was somewhat confusing to read through and understand, but it boils down to two steps:
OptionArgParser::ToBoolean. If that works, then we're good to go.llvm::to_integerto see if it's an integer. If it parses to 0 or 1, we're good.Instead of having an integer out param and a bool return value, the interface is better served with an optional -- Either it parses into true or false, or you get back nothing (nullopt).