https://github.com/bulbazord updated https://github.com/llvm/llvm-project/pull/79901
>From 3cd6afa16fb8b282712b1d3cf103abbd3329fc17 Mon Sep 17 00:00:00 2001 From: Alex Langford <alangf...@apple.com> Date: Mon, 29 Jan 2024 13:15:24 -0800 Subject: [PATCH 1/3] [lldb][NFCI] Minor refactor to CommandObjectProcessHandle::VerifyCommandOptionValue 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). --- lldb/source/Commands/CommandObjectProcess.cpp | 103 ++++++++---------- 1 file changed, 48 insertions(+), 55 deletions(-) diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp index c7b874d197937..f089a86275dc6 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); >From a93faa93b4f743f9b153909b193407a3078ce204 Mon Sep 17 00:00:00 2001 From: Alex Langford <alangf...@apple.com> Date: Mon, 29 Jan 2024 13:29:23 -0800 Subject: [PATCH 2/3] Formatting --- lldb/source/Commands/CommandObjectProcess.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp index f089a86275dc6..890c7314ba7bf 100644 --- a/lldb/source/Commands/CommandObjectProcess.cpp +++ b/lldb/source/Commands/CommandObjectProcess.cpp @@ -1765,9 +1765,9 @@ class CommandObjectProcessHandle : public CommandObjectParsed { // 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 >From 8e784550e50d9f93f3f02b75ecfdf60a977a2f80 Mon Sep 17 00:00:00 2001 From: Alex Langford <alangf...@apple.com> Date: Mon, 29 Jan 2024 17:15:49 -0800 Subject: [PATCH 3/3] [lldb] Remove VerifyCommandOptionValue entirely --- lldb/source/Commands/CommandObjectProcess.cpp | 73 +++++++++---------- 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp index 890c7314ba7bf..722b0e0c376be 100644 --- a/lldb/source/Commands/CommandObjectProcess.cpp +++ b/lldb/source/Commands/CommandObjectProcess.cpp @@ -1591,26 +1591,6 @@ class CommandObjectProcessHandle : public CommandObjectParsed { Options *GetOptions() override { return &m_options; } - 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; - - return parsed_value == 0 ? false : true; - } - - return std::nullopt; - } - void PrintSignalHeader(Stream &str) { str.Printf("NAME PASS STOP NOTIFY\n"); str.Printf("=========== ===== ===== ======\n"); @@ -1666,27 +1646,48 @@ class CommandObjectProcessHandle : public CommandObjectParsed { // the user's options. ProcessSP process_sp = target.GetProcessSP(); - 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); + std::optional<bool> stop_action = {}; + std::optional<bool> pass_action = {}; + std::optional<bool> notify_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.stop.empty()) { + bool success = false; + bool value = OptionArgParser::ToBoolean(m_options.stop, false, &success); + if (!success) { + result.AppendError( + "Invalid argument for command option --stop; must be " + "true or false.\n"); + return; + } + + stop_action = value; } - 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()) { + bool success = false; + bool value = OptionArgParser::ToBoolean(m_options.pass, false, &success); + if (!success) { + result.AppendError( + "Invalid argument for command option --pass; must be " + "true or false.\n"); + return; + } + pass_action = value; + } + + if (!m_options.notify.empty()) { + bool success = false; + bool value = + OptionArgParser::ToBoolean(m_options.notify, false, &success); + if (!success) { + result.AppendError("Invalid argument for command option --notify; must " + "be true or false.\n"); + return; + } + notify_action = value; } 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.has_value() && !pass_action.has_value() && @@ -1728,8 +1729,6 @@ class CommandObjectProcessHandle : public CommandObjectParsed { if (signals_sp) { int32_t signo = signals_sp->GetSignalNumberFromName(arg.c_str()); 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.has_value()) signals_sp->SetShouldStop(signo, *stop_action); if (pass_action.has_value()) { _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits