Author: Alex Langford Date: 2024-02-21T19:26:43-08:00 New Revision: 7e1432f1258e229a4fcc9c017937166f0578e1f8
URL: https://github.com/llvm/llvm-project/commit/7e1432f1258e229a4fcc9c017937166f0578e1f8 DIFF: https://github.com/llvm/llvm-project/commit/7e1432f1258e229a4fcc9c017937166f0578e1f8.diff LOG: [lldb] Standardize command option parsing error messages (#82273) I have been looking to simplify parsing logic and improve the interfaces so that they are both easier to use and harder to abuse. To be specific, I am referring to functions such as `OptionArgParser::ToBoolean`: I would like to go from its current interface to something more like `llvm::Error<bool> ToBoolean(llvm::StringRef option_arg)`. Through working on that, I encountered 2 inconveniences: 1. Option parsing code is not uniform. Every function writes a slightly different error message, so incorporating an error message from the `ToBoolean` implementation is going to be laborious as I figure out what exactly needs to change or stay the same. 2. Changing the interface of `ToBoolean` would require a global atomic change across all of the Command code. This would be quite frustrating to do because of the non-uniformity of our existing code. To address these frustrations, I think it would be easiest to first standardize the error reporting mechanism when parsing options in commands. I do so by introducing `CreateOptionParsingError` which will create an error message of the shape: Invalid value ('${option_arg}') for -${short_value} ('${long_value}'): ${additional_context} Concretely, it would look something like this: (lldb) breakpoint set -n main -G yay error: Invalid value ('yay') for -G (auto-continue): Failed to parse as boolean After this, updating the interfaces for parsing the values themselves should become simpler. Because this can be adopted incrementally, this should be able to done over the course of time instead of all at once as a giant difficult-to-review change. I've changed exactly one function where this function would be used as an illustration of what I am proposing. Added: lldb/unittests/Interpreter/TestOptions.cpp Modified: lldb/include/lldb/Interpreter/Options.h lldb/source/Commands/CommandObjectBreakpoint.cpp lldb/source/Interpreter/Options.cpp lldb/unittests/Interpreter/CMakeLists.txt Removed: ################################################################################ diff --git a/lldb/include/lldb/Interpreter/Options.h b/lldb/include/lldb/Interpreter/Options.h index bf74927cf99db8..18a87e49deee5c 100644 --- a/lldb/include/lldb/Interpreter/Options.h +++ b/lldb/include/lldb/Interpreter/Options.h @@ -336,6 +336,39 @@ class OptionGroupOptions : public Options { bool m_did_finalize = false; }; +/// Creates an error that represents the failure to parse an command line option +/// argument. This creates an error containing all information needed to show +/// the developer what went wrong when parsing their command. It is recommended +/// to use this instead of writing an error by hand. +/// +/// \param[in] option_arg +/// The argument that was attempted to be parsed. +/// +/// \param[in] short_option +/// The short form of the option. For example, if the flag is -f, the short +/// option is "f". +/// +/// \param[in] long_option +/// The long form of the option. This field is optional. If the flag is +/// --force, then the long option is "force". +/// +/// \param[in] additional_context +/// This is extra context that will get included in the error. This field is +/// optional. +/// +/// \return +/// An llvm::Error that contains a standardized format for what went wrong +/// when parsing and why. +llvm::Error CreateOptionParsingError(llvm::StringRef option_arg, + const char short_option, + llvm::StringRef long_option = {}, + llvm::StringRef additional_context = {}); + +static constexpr llvm::StringLiteral g_bool_parsing_error_message = + "Failed to parse as boolean"; +static constexpr llvm::StringLiteral g_int_parsing_error_message = + "Failed to parse as integer"; + } // namespace lldb_private #endif // LLDB_INTERPRETER_OPTIONS_H diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp index 3fdf5cd3cd43d2..fc2217608a0bb9 100644 --- a/lldb/source/Commands/CommandObjectBreakpoint.cpp +++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp @@ -64,6 +64,8 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup { Status error; const int short_option = g_breakpoint_modify_options[option_idx].short_option; + const char *long_option = + g_breakpoint_modify_options[option_idx].long_option; switch (short_option) { case 'c': @@ -84,18 +86,17 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup { case 'G': { bool value, success; value = OptionArgParser::ToBoolean(option_arg, false, &success); - if (success) { + if (success) m_bp_opts.SetAutoContinue(value); - } else - error.SetErrorStringWithFormat( - "invalid boolean value '%s' passed for -G option", - option_arg.str().c_str()); + else + error = CreateOptionParsingError(option_arg, short_option, long_option, + g_bool_parsing_error_message); } break; case 'i': { uint32_t ignore_count; if (option_arg.getAsInteger(0, ignore_count)) - error.SetErrorStringWithFormat("invalid ignore count '%s'", - option_arg.str().c_str()); + error = CreateOptionParsingError(option_arg, short_option, long_option, + g_int_parsing_error_message); else m_bp_opts.SetIgnoreCount(ignore_count); } break; @@ -105,27 +106,29 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup { if (success) { m_bp_opts.SetOneShot(value); } else - error.SetErrorStringWithFormat( - "invalid boolean value '%s' passed for -o option", - option_arg.str().c_str()); + error = CreateOptionParsingError(option_arg, short_option, long_option, + g_bool_parsing_error_message); } break; case 't': { lldb::tid_t thread_id = LLDB_INVALID_THREAD_ID; if (option_arg == "current") { if (!execution_context) { - error.SetErrorStringWithFormat("No context to determine current " - "thread"); + error = CreateOptionParsingError( + option_arg, short_option, long_option, + "No context to determine current thread"); } else { ThreadSP ctx_thread_sp = execution_context->GetThreadSP(); if (!ctx_thread_sp || !ctx_thread_sp->IsValid()) { - error.SetErrorStringWithFormat("No currently selected thread"); + error = + CreateOptionParsingError(option_arg, short_option, long_option, + "No currently selected thread"); } else { thread_id = ctx_thread_sp->GetID(); } } } else if (option_arg.getAsInteger(0, thread_id)) { - error.SetErrorStringWithFormat("invalid thread id string '%s'", - option_arg.str().c_str()); + error = CreateOptionParsingError(option_arg, short_option, long_option, + g_int_parsing_error_message); } if (thread_id != LLDB_INVALID_THREAD_ID) m_bp_opts.SetThreadID(thread_id); @@ -139,8 +142,8 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup { case 'x': { uint32_t thread_index = UINT32_MAX; if (option_arg.getAsInteger(0, thread_index)) { - error.SetErrorStringWithFormat("invalid thread index string '%s'", - option_arg.str().c_str()); + error = CreateOptionParsingError(option_arg, short_option, long_option, + g_int_parsing_error_message); } else { m_bp_opts.GetThreadSpec()->SetIndex(thread_index); } diff --git a/lldb/source/Interpreter/Options.cpp b/lldb/source/Interpreter/Options.cpp index 89fe69009d9036..51b7e6b26b6efa 100644 --- a/lldb/source/Interpreter/Options.cpp +++ b/lldb/source/Interpreter/Options.cpp @@ -1365,3 +1365,16 @@ llvm::Expected<Args> Options::Parse(const Args &args, argv.erase(argv.begin(), argv.begin() + OptionParser::GetOptionIndex()); return ReconstituteArgsAfterParsing(argv, args); } + +llvm::Error lldb_private::CreateOptionParsingError( + llvm::StringRef option_arg, const char short_option, + llvm::StringRef long_option, llvm::StringRef additional_context) { + std::string buffer; + llvm::raw_string_ostream stream(buffer); + stream << "Invalid value ('" << option_arg << "') for -" << short_option; + if (!long_option.empty()) + stream << " (" << long_option << ")"; + if (!additional_context.empty()) + stream << ": " << additional_context; + return llvm::createStringError(llvm::inconvertibleErrorCode(), buffer); +} diff --git a/lldb/unittests/Interpreter/CMakeLists.txt b/lldb/unittests/Interpreter/CMakeLists.txt index 5b5268ffe97321..54cea995084d3d 100644 --- a/lldb/unittests/Interpreter/CMakeLists.txt +++ b/lldb/unittests/Interpreter/CMakeLists.txt @@ -2,6 +2,7 @@ add_lldb_unittest(InterpreterTests TestCommandPaths.cpp TestCompletion.cpp TestOptionArgParser.cpp + TestOptions.cpp TestOptionValue.cpp TestOptionValueFileColonLine.cpp TestRegexCommand.cpp diff --git a/lldb/unittests/Interpreter/TestOptions.cpp b/lldb/unittests/Interpreter/TestOptions.cpp new file mode 100644 index 00000000000000..93474e3c5713c3 --- /dev/null +++ b/lldb/unittests/Interpreter/TestOptions.cpp @@ -0,0 +1,29 @@ +//===-- TestOptions.cpp ---------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "lldb/Interpreter/Options.h" +#include "gtest/gtest.h" + +#include "llvm/Testing/Support/Error.h" + +using namespace lldb_private; + +TEST(OptionsTest, CreateOptionParsingError) { + ASSERT_THAT_ERROR( + CreateOptionParsingError("yippee", 'f', "fun", + "unable to convert 'yippee' to boolean"), + llvm::FailedWithMessage("Invalid value ('yippee') for -f (fun): unable " + "to convert 'yippee' to boolean")); + + ASSERT_THAT_ERROR( + CreateOptionParsingError("52", 'b', "bean-count"), + llvm::FailedWithMessage("Invalid value ('52') for -b (bean-count)")); + + ASSERT_THAT_ERROR(CreateOptionParsingError("c", 'm'), + llvm::FailedWithMessage("Invalid value ('c') for -m")); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits