Add a createError variant without error code (NFC) #93209
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb @llvm/pr-subscribers-llvm-support Author: Adrian Prantl (adrian-prantl) ChangesFor the significant amount of call sites that want to create an incontrovertible error, such a wrapper function creates a significant readability improvement and lowers the cost of entry to add error handling in more places. Patch is 21.63 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/93209.diff 13 Files Affected:
diff --git a/lldb/source/Host/common/Socket.cpp b/lldb/source/Host/common/Socket.cpp
index bd0c127a08956..e060c174326b8 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -87,8 +87,8 @@ llvm::Error Socket::Initialize() {
if (err == 0) {
if (wsaData.wVersion < wVersion) {
WSACleanup();
- return llvm::make_error<llvm::StringError>(
- "WSASock version is not expected.", llvm::inconvertibleErrorCode());
+ return llvm::createStringError("WSASock version is not expected.",
+ llvm::inconvertibleErrorCode());
}
} else {
return llvm::errorCodeToError(llvm::mapWindowsError(::WSAGetLastError()));
diff --git a/lldb/source/Interpreter/Options.cpp b/lldb/source/Interpreter/Options.cpp
index 51b7e6b26b6ef..4e7d074ace1b8 100644
--- a/lldb/source/Interpreter/Options.cpp
+++ b/lldb/source/Interpreter/Options.cpp
@@ -931,8 +931,7 @@ llvm::Expected<Args> Options::ParseAlias(const Args &args,
Option *long_options = GetLongOptions();
if (long_options == nullptr) {
- return llvm::make_error<llvm::StringError>("Invalid long options",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError("Invalid long options");
}
std::string short_options = BuildShortOptions(long_options);
@@ -957,8 +956,7 @@ llvm::Expected<Args> Options::ParseAlias(const Args &args,
break;
if (val == '?') {
- return llvm::make_error<llvm::StringError>(
- "Unknown or ambiguous option", llvm::inconvertibleErrorCode());
+ return llvm::createStringError("Unknown or ambiguous option");
}
if (val == 0)
@@ -980,9 +978,8 @@ llvm::Expected<Args> Options::ParseAlias(const Args &args,
// See if the option takes an argument, and see if one was supplied.
if (long_options_index == -1) {
- return llvm::make_error<llvm::StringError>(
- llvm::formatv("Invalid option with value '{0}'.", char(val)).str(),
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError(
+ llvm::formatv("Invalid option with value '{0}'.", char(val)).str());
}
StreamString option_str;
@@ -995,11 +992,10 @@ llvm::Expected<Args> Options::ParseAlias(const Args &args,
switch (has_arg) {
case OptionParser::eRequiredArgument:
if (OptionParser::GetOptionArgument() == nullptr) {
- return llvm::make_error<llvm::StringError>(
+ return llvm::createStringError(
llvm::formatv("Option '{0}' is missing argument specifier.",
option_str.GetString())
- .str(),
- llvm::inconvertibleErrorCode());
+ .str());
}
[[fallthrough]];
case OptionParser::eOptionalArgument:
@@ -1008,12 +1004,11 @@ llvm::Expected<Args> Options::ParseAlias(const Args &args,
case OptionParser::eNoArgument:
break;
default:
- return llvm::make_error<llvm::StringError>(
+ return llvm::createStringError(
llvm::formatv("error with options table; invalid value in has_arg "
"field for option '{0}'.",
char(val))
- .str(),
- llvm::inconvertibleErrorCode());
+ .str());
}
// Find option in the argument list; also see if it was supposed to take an
// argument and if one was supplied. Remove option (and argument, if
@@ -1261,8 +1256,7 @@ llvm::Expected<Args> Options::Parse(const Args &args,
Status error;
Option *long_options = GetLongOptions();
if (long_options == nullptr) {
- return llvm::make_error<llvm::StringError>("Invalid long options.",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError("Invalid long options.");
}
std::string short_options = BuildShortOptions(long_options);
@@ -1322,9 +1316,8 @@ llvm::Expected<Args> Options::Parse(const Args &args,
if (!platform_sp && require_validation) {
// Caller requires validation but we cannot validate as we don't have
// the mandatory platform against which to validate.
- return llvm::make_error<llvm::StringError>(
- "cannot validate options: no platform available",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError(
+ "cannot validate options: no platform available");
}
bool validation_failed = false;
diff --git a/lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc64.cpp b/lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc64.cpp
index 3d9b4566ca1c9..b96f9d4deb37c 100644
--- a/lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc64.cpp
+++ b/lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc64.cpp
@@ -501,14 +501,12 @@ class ReturnValueExtractor {
CompilerType &type) {
RegisterContext *reg_ctx = thread.GetRegisterContext().get();
if (!reg_ctx)
- return llvm::make_error<llvm::StringError>(
- LOG_PREFIX "Failed to get RegisterContext",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError(LOG_PREFIX
+ "Failed to get RegisterContext");
ProcessSP process_sp = thread.GetProcess();
if (!process_sp)
- return llvm::make_error<llvm::StringError>(
- LOG_PREFIX "GetProcess() failed", llvm::inconvertibleErrorCode());
+ return llvm::createStringError(LOG_PREFIX "GetProcess() failed");
return ReturnValueExtractor(thread, type, reg_ctx, process_sp);
}
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
index ce52f35952478..6e676de146b3d 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -2494,8 +2494,7 @@ bool ScriptInterpreterPythonImpl::LoadScriptingModule(
auto ExtendSysPath = [&](std::string directory) -> llvm::Error {
if (directory.empty()) {
- return llvm::make_error<llvm::StringError>(
- "invalid directory name", llvm::inconvertibleErrorCode());
+ return llvm::createStringError("invalid directory name");
}
replace_all(directory, "\\", "\\\\");
@@ -2508,10 +2507,8 @@ bool ScriptInterpreterPythonImpl::LoadScriptingModule(
directory.c_str(), directory.c_str());
bool syspath_retval =
ExecuteMultipleLines(command_stream.GetData(), exc_options).Success();
- if (!syspath_retval) {
- return llvm::make_error<llvm::StringError>(
- "Python sys.path handling failed", llvm::inconvertibleErrorCode());
- }
+ if (!syspath_retval)
+ return llvm::createStringError("Python sys.path handling failed");
return llvm::Error::success();
};
diff --git a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h
index 83215bf3c87e4..041b388f9f344 100644
--- a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h
+++ b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h
@@ -120,9 +120,8 @@ class SymbolFileBreakpad : public SymbolFileCommon {
llvm::Expected<lldb::TypeSystemSP>
GetTypeSystemForLanguage(lldb::LanguageType language) override {
- return llvm::make_error<llvm::StringError>(
- "SymbolFileBreakpad does not support GetTypeSystemForLanguage",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError(
+ "SymbolFileBreakpad does not support GetTypeSystemForLanguage");
}
CompilerDeclContext FindNamespace(ConstString name,
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 1b2235b4b2b5b..369ae46cf264a 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5272,8 +5272,7 @@ TypeSystemClang::GetNumChildren(lldb::opaque_compiler_type_t type,
bool omit_empty_base_classes,
const ExecutionContext *exe_ctx) {
if (!type)
- return llvm::make_error<llvm::StringError>("invalid clang type",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError("invalid clang type");
uint32_t num_children = 0;
clang::QualType qual_type(RemoveWrappingTypes(GetQualType(type)));
@@ -5331,9 +5330,8 @@ TypeSystemClang::GetNumChildren(lldb::opaque_compiler_type_t type,
num_children += std::distance(record_decl->field_begin(),
record_decl->field_end());
} else
- return llvm::make_error<llvm::StringError>(
- "incomplete type \"" + GetDisplayTypeName(type).GetString() + "\"",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError(
+ "incomplete type \"" + GetDisplayTypeName(type).GetString() + "\"");
break;
case clang::Type::ObjCObject:
case clang::Type::ObjCInterface:
@@ -6239,9 +6237,7 @@ llvm::Expected<CompilerType> TypeSystemClang::GetChildCompilerTypeAtIndex(
std::optional<uint64_t> size =
base_class_clang_type.GetBitSize(get_exe_scope());
if (!size)
- return llvm::make_error<llvm::StringError>(
- "no size info for base class",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError("no size info for base class");
uint64_t base_class_clang_type_bit_size = *size;
@@ -6274,8 +6270,7 @@ llvm::Expected<CompilerType> TypeSystemClang::GetChildCompilerTypeAtIndex(
std::optional<uint64_t> size =
field_clang_type.GetByteSize(get_exe_scope());
if (!size)
- return llvm::make_error<llvm::StringError>(
- "no size info for field", llvm::inconvertibleErrorCode());
+ return llvm::createStringError("no size info for field");
child_byte_size = *size;
const uint32_t child_bit_size = child_byte_size * 8;
diff --git a/lldb/source/Symbol/CompilerType.cpp b/lldb/source/Symbol/CompilerType.cpp
index b5269cf66235c..f8da9ef7b7640 100644
--- a/lldb/source/Symbol/CompilerType.cpp
+++ b/lldb/source/Symbol/CompilerType.cpp
@@ -805,8 +805,7 @@ CompilerType::GetNumChildren(bool omit_empty_base_classes,
if (auto type_system_sp = GetTypeSystem())
return type_system_sp->GetNumChildren(m_type, omit_empty_base_classes,
exe_ctx);
- return llvm::make_error<llvm::StringError>("invalid type",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError("invalid type");
}
lldb::BasicType CompilerType::GetBasicTypeEnumeration() const {
diff --git a/lldb/source/Symbol/Symbol.cpp b/lldb/source/Symbol/Symbol.cpp
index 1895f299cc06a..9b0042ffdb4bf 100644
--- a/lldb/source/Symbol/Symbol.cpp
+++ b/lldb/source/Symbol/Symbol.cpp
@@ -101,18 +101,15 @@ const Symbol &Symbol::operator=(const Symbol &rhs) {
llvm::Expected<Symbol> Symbol::FromJSON(const JSONSymbol &symbol,
SectionList *section_list) {
if (!section_list)
- return llvm::make_error<llvm::StringError>("no section list provided",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError("no section list provided");
if (!symbol.value && !symbol.address)
- return llvm::make_error<llvm::StringError>(
- "symbol must contain either a value or an address",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError(
+ "symbol must contain either a value or an address");
if (symbol.value && symbol.address)
- return llvm::make_error<llvm::StringError>(
- "symbol cannot contain both a value and an address",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError(
+ "symbol cannot contain both a value and an address");
const uint64_t size = symbol.size.value_or(0);
const bool is_artificial = false;
@@ -133,9 +130,8 @@ llvm::Expected<Symbol> Symbol::FromJSON(const JSONSymbol &symbol,
AddressRange(section_sp, offset, size), size_is_valid,
contains_linker_annotations, flags);
}
- return llvm::make_error<llvm::StringError>(
- llvm::formatv("no section found for address: {0:x}", *symbol.address),
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError(
+ llvm::formatv("no section found for address: {0:x}", *symbol.address));
}
// Absolute symbols encode the integer value in the m_offset of the
diff --git a/lldb/source/Symbol/SymbolFileOnDemand.cpp b/lldb/source/Symbol/SymbolFileOnDemand.cpp
index c6d9f0071c392..0cfe9fc1514b5 100644
--- a/lldb/source/Symbol/SymbolFileOnDemand.cpp
+++ b/lldb/source/Symbol/SymbolFileOnDemand.cpp
@@ -457,9 +457,8 @@ SymbolFileOnDemand::GetTypeSystemForLanguage(LanguageType language) {
Log *log = GetLog();
LLDB_LOG(log, "[{0}] {1} is skipped for language type {2}",
GetSymbolFileName(), __FUNCTION__, language);
- return llvm::make_error<llvm::StringError>(
- "GetTypeSystemForLanguage is skipped by SymbolFileOnDemand",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError(
+ "GetTypeSystemForLanguage is skipped by SymbolFileOnDemand");
}
return m_sym_file_impl->GetTypeSystemForLanguage(language);
}
diff --git a/lldb/source/Symbol/TypeSystem.cpp b/lldb/source/Symbol/TypeSystem.cpp
index 3665771b18890..4956f10a0b0a7 100644
--- a/lldb/source/Symbol/TypeSystem.cpp
+++ b/lldb/source/Symbol/TypeSystem.cpp
@@ -267,9 +267,8 @@ llvm::Expected<lldb::TypeSystemSP> TypeSystemMap::GetTypeSystemForLanguage(
std::optional<CreateCallback> create_callback) {
std::lock_guard<std::mutex> guard(m_mutex);
if (m_clear_in_progress)
- return llvm::make_error<llvm::StringError>(
- "Unable to get TypeSystem because TypeSystemMap is being cleared",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError(
+ "Unable to get TypeSystem because TypeSystemMap is being cleared");
collection::iterator pos = m_map.find(language);
if (pos != m_map.end()) {
@@ -277,11 +276,10 @@ llvm::Expected<lldb::TypeSystemSP> TypeSystemMap::GetTypeSystemForLanguage(
assert(!pos->second->weak_from_this().expired());
return pos->second;
}
- return llvm::make_error<llvm::StringError>(
+ return llvm::createStringError(
"TypeSystem for language " +
- llvm::StringRef(Language::GetNameForLanguageType(language)) +
- " doesn't exist",
- llvm::inconvertibleErrorCode());
+ llvm::StringRef(Language::GetNameForLanguageType(language)) +
+ " doesn't exist");
}
for (const auto &pair : m_map) {
@@ -291,31 +289,27 @@ llvm::Expected<lldb::TypeSystemSP> TypeSystemMap::GetTypeSystemForLanguage(
m_map[language] = pair.second;
if (pair.second)
return pair.second;
- return llvm::make_error<llvm::StringError>(
+ return llvm::createStringError(
"TypeSystem for language " +
- llvm::StringRef(Language::GetNameForLanguageType(language)) +
- " doesn't exist",
- llvm::inconvertibleErrorCode());
+ llvm::StringRef(Language::GetNameForLanguageType(language)) +
+ " doesn't exist");
}
}
if (!create_callback)
- return llvm::make_error<llvm::StringError>(
+ return llvm::createStringError(
"Unable to find type system for language " +
- llvm::StringRef(Language::GetNameForLanguageType(language)),
- llvm::inconvertibleErrorCode());
-
+ llvm::StringRef(Language::GetNameForLanguageType(language)));
// Cache even if we get a shared pointer that contains a null type system
// back.
TypeSystemSP type_system_sp = (*create_callback)();
m_map[language] = type_system_sp;
if (type_system_sp)
return type_system_sp;
- return llvm::make_error<llvm::StringError>(
+ return llvm::createStringError(
"TypeSystem for language " +
- llvm::StringRef(Language::GetNameForLanguageType(language)) +
- " doesn't exist",
- llvm::inconvertibleErrorCode());
+ llvm::StringRef(Language::GetNameForLanguageType(language)) +
+ " doesn't exist");
}
llvm::Expected<lldb::TypeSystemSP>
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 77731167995e1..ec0da8a1378a8 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -2414,8 +2414,7 @@ llvm::Expected<lldb::TypeSystemSP>
Target::GetScratchTypeSystemForLanguage(lldb::LanguageType language,
bool create_on_demand) {
if (!m_valid)
- return llvm::make_error<llvm::StringError>("Invalid Target",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError("Invalid Target");
if (language == eLanguageTypeMipsAssembler // GNU AS and LLVM use it for all
// assembly code
@@ -2428,9 +2427,8 @@ Target::GetScratchTypeSystemForLanguage(lldb::LanguageType language,
// target language.
} else {
if (languages_for_expressions.Empty())
- return llvm::make_error<llvm::StringError>(
- "No expression support for any languages",
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError(
+ "No expression support for any languages");
language = (LanguageType)languages_for_expressions.bitvector.find_first();
}
}
@@ -2574,23 +2572,20 @@ Target::CreateUtilityFunction(std::string expression, std::string name,
return type_system_or_err.takeError();
auto ts = *type_system_or_err;
if (!ts)
- return llvm::make_error<llvm::StringError>(
+ return llvm::createStringError(
llvm::StringRef("Type system for language ") +
- Language::GetNameForLanguageType(language) +
- llvm::StringRef(" is no longer live"),
- llvm::inconvertibleErrorCode());
+ Language::GetNameForLanguageType(language) +
+ llvm::StringRef(" is no longer live"));
std::unique_ptr<UtilityFunction> utility_fn =
ts->CreateUtilityFunction(std::move(expression), std::move(name));
if (!utility_fn)
- return llvm::make_error<llvm::StringError>(
+ return llvm::createStringError(
llvm::StringRef("Could not create an expression for language") +
- Language::GetNameForLanguageType(language),
- llvm::inconvertibleErrorCode());
+ Language::GetNameForLanguageType(language));
DiagnosticManager diagnostics;
if (!utility_fn->Install(diagnostics, exe_ctx))
- return llvm::make_error<llvm::StringError>(diagnostics.GetString(),
- llvm::inconvertibleErrorCode());
+ return llvm::createStringError(diagnostics.GetString());
return std::move(utility_fn);
}
@@ -2621,8 +2616,7 @@ void Target::SetDefaultArchitecture(const ArchSpec &arch) {
llvm::Error Target::SetLabel(llvm::StringRef label) {
size_t n = LLDB_INVALID_INDEX32;
if (llvm::to_integer(label, n))
- return llvm::make_error<llvm::StringError>(
- "Cannot use integer as target label.", llvm::inconvertibleErrorCode());
+ return llvm::createStringError("Cannot use integer as target label.");
TargetList &targets = GetDebugger().GetTargetList();
for (size_t i = 0; i < targets.GetNumTargets(); i++) {
TargetSP target_sp = targets.GetTargetAtIndex(i);
@@ -2790,15 +2784,13 @@ llvm::Expected<lldb_private::Address> Target::GetEntryPointAddress() {
// We haven't found the entry point address. Return an appropriate error.
if (!has_primary_executable)
- return llvm::make_error<llvm::StringError>(
+ return llvm::createStringError(
"No primary executable found and could not find entry point address in "
- "any executable module",
- llvm::inconvertibleErrorCode());
+ "any exe...
[truncated]
|
|
The obvious downside is that it makes it easier to not even try to find a matching error code, but it looks like most incontrovertible errors are caught before the get reported as an exit status. |
There was a problem hiding this comment.
I'm in favor of this. I rarely have a meaningful error code and it makes the code needlessly verbose.
I was going to ask if we wanted to make an overload of the existing createStringError but without the error code. But then I noticed that it uses the printf-style formatters (and not the llvm one). I think @bulbazord tried to add a formatv overload but that slowed down compilation of LLVM itself. Long story short, I think having this function take an llvm::Twine is fine.
adrian-prantl
force-pushed
the
incontrovertible-string-errors
branch
from
c579798 to
bece445
Compare
For the significant amount of call sites that want to create an incontrovertible error, such a wrapper function creates a significant readability improvement and lowers the cost of entry to add error handling in more places.
adrian-prantl
force-pushed
the
incontrovertible-string-errors
branch
from
bece445 to
c573859
Compare
medismailben
approved these changes
May 23, 2024
adrian-prantl
added a commit
to adrian-prantl/llvm-project
that referenced
this pull request
May 24, 2024
For the significant amount of call sites that want to create an incontrovertible error, such a wrapper function creates a significant readability improvement and lowers the cost of entry to add error handling in more places. (cherry picked from commit af31883)
|
Late to the party here, but some context might be of interest:
In practice (1) nobody has adhered to the documentation rule (me included), and (2) nobody seems to be converting errors back into std::error_codes in a dangerous way*. At this point I'd be fine getting rid of
|
|
Thanks for providing the context, Lang! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For the significant amount of call sites that want to create an incontrovertible error, such a wrapper function creates a significant readability improvement and lowers the cost of entry to add error handling in more places.