[lldb] Turn lldb_private::Status into a value type. #106163
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 Author: Adrian Prantl (adrian-prantl) ChangesThis patch removes all of the Set.* methods from Status. This cleanup is part of a series of patches that make it harder use the anti-pattern of keeping a long-lives Status object around and updating it while dropping any errors it contains on the floor. This patch is largely NFC, the more interesting next steps this enables is to:
Note that step (2) will bring 90% of the benefits for users, and step (3) will dramatically clean up the error handling code in various places. In the end my goal is to convert all APIs that are of the form
The interesting changes are in Status.h and Status.cpp, all other changes are mostly
Patch is 904.59 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106163.diff 260 Files Affected:
diff --git a/lldb/bindings/python/python-wrapper.swig b/lldb/bindings/python/python-wrapper.swig
index 360c392235a866..810673aaec5d19 100644
--- a/lldb/bindings/python/python-wrapper.swig
+++ b/lldb/bindings/python/python-wrapper.swig
@@ -306,11 +306,11 @@ PythonObject lldb_private::python::SWIGBridge::LLDBSwigPythonCreateScriptedStopH
const char *session_dictionary_name, const StructuredDataImpl &args_impl,
Status &error) {
if (python_class_name == NULL || python_class_name[0] == '\0') {
- error.SetErrorString("Empty class name.");
+ error = Status::FromErrorString("Empty class name.");
return PythonObject();
}
if (!session_dictionary_name) {
- error.SetErrorString("No session dictionary");
+ error = Status::FromErrorString("No session dictionary");
return PythonObject();
}
@@ -322,7 +322,7 @@ PythonObject lldb_private::python::SWIGBridge::LLDBSwigPythonCreateScriptedStopH
python_class_name, dict);
if (!pfunc.IsAllocated()) {
- error.SetErrorStringWithFormat("Could not find class: %s.",
+ error = Status::FromErrorStringWithFormat("Could not find class: %s.",
python_class_name);
return PythonObject();
}
@@ -337,7 +337,7 @@ PythonObject lldb_private::python::SWIGBridge::LLDBSwigPythonCreateScriptedStopH
if (auto args_info = callback_func.GetArgInfo()) {
size_t num_args = (*args_info).max_positional_args;
if (num_args != 2) {
- error.SetErrorStringWithFormat(
+ error = Status::FromErrorStringWithFormat(
"Wrong number of args for "
"handle_stop callback, should be 2 (excluding self), got: %zu",
num_args);
@@ -345,15 +345,17 @@ PythonObject lldb_private::python::SWIGBridge::LLDBSwigPythonCreateScriptedStopH
} else
return result;
} else {
- error.SetErrorString("Couldn't get num arguments for handle_stop "
- "callback.");
+ error = Status::FromErrorString(
+ "Couldn't get num arguments for handle_stop "
+ "callback.");
return PythonObject();
}
return result;
} else {
- error.SetErrorStringWithFormat("Class \"%s\" is missing the required "
- "handle_stop callback.",
- python_class_name);
+ error = Status::FromErrorStringWithFormat(
+ "Class \"%s\" is missing the required "
+ "handle_stop callback.",
+ python_class_name);
}
}
return PythonObject();
diff --git a/lldb/include/lldb/Core/StructuredDataImpl.h b/lldb/include/lldb/Core/StructuredDataImpl.h
index 0e068b4598cdda..fd0a7b94d3a6cf 100644
--- a/lldb/include/lldb/Core/StructuredDataImpl.h
+++ b/lldb/include/lldb/Core/StructuredDataImpl.h
@@ -50,38 +50,28 @@ class StructuredDataImpl {
}
Status GetAsJSON(Stream &stream) const {
- Status error;
-
- if (!m_data_sp) {
- error.SetErrorString("No structured data.");
- return error;
- }
+ if (!m_data_sp)
+ return Status::FromErrorString("No structured data.");
llvm::json::OStream s(stream.AsRawOstream());
m_data_sp->Serialize(s);
- return error;
+ return Status();
}
Status GetDescription(Stream &stream) const {
- Status error;
-
- if (!m_data_sp) {
- error.SetErrorString("Cannot pretty print structured data: "
- "no data to print.");
- return error;
- }
+ if (!m_data_sp)
+ return Status::FromErrorString("Cannot pretty print structured data: "
+ "no data to print.");
// Grab the plugin
lldb::StructuredDataPluginSP plugin_sp = m_plugin_wp.lock();
// If there's no plugin, call underlying data's dump method:
if (!plugin_sp) {
- if (!m_data_sp) {
- error.SetErrorString("No data to describe.");
- return error;
- }
+ if (!m_data_sp)
+ return Status::FromErrorString("No data to describe.");
m_data_sp->GetDescription(stream);
- return error;
+ return Status();
}
// Get the data's description.
return plugin_sp->GetDescription(m_data_sp, stream);
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h
index 3850edf879ac45..790229bf1b0639 100644
--- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h
+++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h
@@ -48,7 +48,7 @@ class ScriptedInterface {
llvm::Twine(llvm::Twine(" (") + llvm::Twine(detailed_error) +
llvm::Twine(")"))
.str();
- error.SetErrorString(full_error_message);
+ error = Status(full_error_message);
return {};
}
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h
index 7feaa01fe89b86..ac349de57c143d 100644
--- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h
+++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h
@@ -31,15 +31,18 @@ class ScriptedPlatformInterface : virtual public ScriptedInterface {
}
virtual Status AttachToProcess(lldb::ProcessAttachInfoSP attach_info) {
- return Status("ScriptedPlatformInterface cannot attach to a process");
+ return Status::FromErrorString(
+ "ScriptedPlatformInterface cannot attach to a process");
}
virtual Status LaunchProcess(lldb::ProcessLaunchInfoSP launch_info) {
- return Status("ScriptedPlatformInterface cannot launch process");
+ return Status::FromErrorString(
+ "ScriptedPlatformInterface cannot launch process");
}
virtual Status KillProcess(lldb::pid_t pid) {
- return Status("ScriptedPlatformInterface cannot kill process");
+ return Status::FromErrorString(
+ "ScriptedPlatformInterface cannot kill process");
}
};
} // namespace lldb_private
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h
index 10203b1f8baa7a..076ca43d451118 100644
--- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h
+++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h
@@ -29,23 +29,28 @@ class ScriptedProcessInterface : virtual public ScriptedInterface {
virtual StructuredData::DictionarySP GetCapabilities() { return {}; }
virtual Status Attach(const ProcessAttachInfo &attach_info) {
- return Status("ScriptedProcess did not attach");
+ return Status::FromErrorString("ScriptedProcess did not attach");
}
- virtual Status Launch() { return Status("ScriptedProcess did not launch"); }
+ virtual Status Launch() {
+ return Status::FromErrorString("ScriptedProcess did not launch");
+ }
- virtual Status Resume() { return Status("ScriptedProcess did not resume"); }
+ virtual Status Resume() {
+ return Status::FromErrorString("ScriptedProcess did not resume");
+ }
virtual std::optional<MemoryRegionInfo>
GetMemoryRegionContainingAddress(lldb::addr_t address, Status &error) {
- error.SetErrorString("ScriptedProcess have no memory region.");
+ error = Status::FromErrorString("ScriptedProcess have no memory region.");
return {};
}
virtual StructuredData::DictionarySP GetThreadsInfo() { return {}; }
virtual bool CreateBreakpoint(lldb::addr_t addr, Status &error) {
- error.SetErrorString("ScriptedProcess don't support creating breakpoints.");
+ error = Status::FromErrorString(
+ "ScriptedProcess don't support creating breakpoints.");
return {};
}
diff --git a/lldb/include/lldb/Interpreter/OptionValue.h b/lldb/include/lldb/Interpreter/OptionValue.h
index bfbdbead72c66d..d19c8b8fab6222 100644
--- a/lldb/include/lldb/Interpreter/OptionValue.h
+++ b/lldb/include/lldb/Interpreter/OptionValue.h
@@ -119,7 +119,8 @@ class OptionValue {
virtual lldb::OptionValueSP GetSubValue(const ExecutionContext *exe_ctx,
llvm::StringRef name,
Status &error) const {
- error.SetErrorStringWithFormatv("'{0}' is not a valid subvalue", name);
+ error = Status::FromErrorStringWithFormatv("'{0}' is not a valid subvalue",
+ name);
return lldb::OptionValueSP();
}
diff --git a/lldb/include/lldb/Interpreter/ScriptInterpreter.h b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
index 89a480a28880aa..addb1394ab5652 100644
--- a/lldb/include/lldb/Interpreter/ScriptInterpreter.h
+++ b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
@@ -176,25 +176,19 @@ class ScriptInterpreter : public PluginInterface {
virtual Status ExecuteMultipleLines(
const char *in_string,
const ExecuteScriptOptions &options = ExecuteScriptOptions()) {
- Status error;
- error.SetErrorString("not implemented");
- return error;
+ return Status::FromErrorString("not implemented");
}
virtual Status
ExportFunctionDefinitionToInterpreter(StringList &function_def) {
- Status error;
- error.SetErrorString("not implemented");
- return error;
+ return Status::FromErrorString("not implemented");
}
virtual Status GenerateBreakpointCommandCallbackData(StringList &input,
std::string &output,
bool has_extra_args,
bool is_callback) {
- Status error;
- error.SetErrorString("not implemented");
- return error;
+ return Status::FromErrorString("not implemented");
}
virtual bool GenerateWatchpointCommandCallbackData(StringList &input,
@@ -280,8 +274,9 @@ class ScriptInterpreter : public PluginInterface {
virtual StructuredData::GenericSP
CreateScriptedStopHook(lldb::TargetSP target_sp, const char *class_name,
const StructuredDataImpl &args_data, Status &error) {
- error.SetErrorString("Creating scripted stop-hooks with the current "
- "script interpreter is not supported.");
+ error =
+ Status::FromErrorString("Creating scripted stop-hooks with the current "
+ "script interpreter is not supported.");
return StructuredData::GenericSP();
}
@@ -308,9 +303,7 @@ class ScriptInterpreter : public PluginInterface {
virtual Status GenerateFunction(const char *signature,
const StringList &input,
bool is_callback) {
- Status error;
- error.SetErrorString("unimplemented");
- return error;
+ return Status::FromErrorString("not implemented");
}
virtual void CollectDataForBreakpointCommandCallback(
@@ -329,18 +322,14 @@ class ScriptInterpreter : public PluginInterface {
virtual Status SetBreakpointCommandCallback(BreakpointOptions &bp_options,
const char *callback_text,
bool is_callback) {
- Status error;
- error.SetErrorString("unimplemented");
- return error;
+ return Status::FromErrorString("not implemented");
}
/// This one is for deserialization:
virtual Status SetBreakpointCommandCallback(
BreakpointOptions &bp_options,
std::unique_ptr<BreakpointOptions::CommandData> &data_up) {
- Status error;
- error.SetErrorString("unimplemented");
- return error;
+ return Status::FromErrorString("not implemented");
}
Status SetBreakpointCommandCallbackFunction(
@@ -352,9 +341,7 @@ class ScriptInterpreter : public PluginInterface {
SetBreakpointCommandCallbackFunction(BreakpointOptions &bp_options,
const char *function_name,
StructuredData::ObjectSP extra_args_sp) {
- Status error;
- error.SetErrorString("unimplemented");
- return error;
+ return Status::FromErrorString("not implemented");
}
/// Set a one-liner as the callback for the watchpoint.
@@ -453,33 +440,33 @@ class ScriptInterpreter : public PluginInterface {
virtual bool RunScriptFormatKeyword(const char *impl_function,
Process *process, std::string &output,
Status &error) {
- error.SetErrorString("unimplemented");
+ error = Status::FromErrorString("unimplemented");
return false;
}
virtual bool RunScriptFormatKeyword(const char *impl_function, Thread *thread,
std::string &output, Status &error) {
- error.SetErrorString("unimplemented");
+ error = Status::FromErrorString("unimplemented");
return false;
}
virtual bool RunScriptFormatKeyword(const char *impl_function, Target *target,
std::string &output, Status &error) {
- error.SetErrorString("unimplemented");
+ error = Status::FromErrorString("unimplemented");
return false;
}
virtual bool RunScriptFormatKeyword(const char *impl_function,
StackFrame *frame, std::string &output,
Status &error) {
- error.SetErrorString("unimplemented");
+ error = Status::FromErrorString("unimplemented");
return false;
}
virtual bool RunScriptFormatKeyword(const char *impl_function,
ValueObject *value, std::string &output,
Status &error) {
- error.SetErrorString("unimplemented");
+ error = Status::FromErrorString("unimplemented");
return false;
}
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index 9cf6f50558570b..a7de991104434d 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -616,10 +616,8 @@ class Process : public std::enable_shared_from_this<Process>,
virtual Status LoadCore();
virtual Status DoLoadCore() {
- Status error;
- error.SetErrorStringWithFormatv(
+ return Status::FromErrorStringWithFormatv(
"error: {0} does not support loading core files.", GetPluginName());
- return error;
}
/// The "ShadowListener" for a process is just an ordinary Listener that
@@ -990,9 +988,7 @@ class Process : public std::enable_shared_from_this<Process>,
/// \return
/// Returns an error object.
virtual Status DoConnectRemote(llvm::StringRef remote_url) {
- Status error;
- error.SetErrorString("remote connections are not supported");
- return error;
+ return Status::FromErrorString("remote connections are not supported");
}
/// Attach to an existing process using a process ID.
@@ -1011,11 +1007,9 @@ class Process : public std::enable_shared_from_this<Process>,
/// hanming : need flag
virtual Status DoAttachToProcessWithID(lldb::pid_t pid,
const ProcessAttachInfo &attach_info) {
- Status error;
- error.SetErrorStringWithFormatv(
+ return Status::FromErrorStringWithFormatv(
"error: {0} does not support attaching to a process by pid",
GetPluginName());
- return error;
}
/// Attach to an existing process using a partial process name.
@@ -1034,9 +1028,7 @@ class Process : public std::enable_shared_from_this<Process>,
virtual Status
DoAttachToProcessWithName(const char *process_name,
const ProcessAttachInfo &attach_info) {
- Status error;
- error.SetErrorString("attach by name is not supported");
- return error;
+ return Status::FromErrorString("attach by name is not supported");
}
/// Called after attaching a process.
@@ -1101,10 +1093,8 @@ class Process : public std::enable_shared_from_this<Process>,
/// An Status instance indicating success or failure of the
/// operation.
virtual Status DoLaunch(Module *exe_module, ProcessLaunchInfo &launch_info) {
- Status error;
- error.SetErrorStringWithFormatv(
+ return Status::FromErrorStringWithFormatv(
"error: {0} does not support launching processes", GetPluginName());
- return error;
}
/// Called after launching a process.
@@ -1136,10 +1126,8 @@ class Process : public std::enable_shared_from_this<Process>,
/// \see Thread:Step()
/// \see Thread:Suspend()
virtual Status DoResume() {
- Status error;
- error.SetErrorStringWithFormatv(
+ return Status::FromErrorStringWithFormatv(
"error: {0} does not support resuming processes", GetPluginName());
- return error;
}
/// Called after resuming a process.
@@ -1171,10 +1159,8 @@ class Process : public std::enable_shared_from_this<Process>,
/// Returns \b true if the process successfully halts, \b false
/// otherwise.
virtual Status DoHalt(bool &caused_stop) {
- Status error;
- error.SetErrorStringWithFormatv(
+ return Status::FromErrorStringWithFormatv(
"error: {0} does not support halting processes", GetPluginName());
- return error;
}
/// Called after halting a process.
@@ -1197,11 +1183,9 @@ class Process : public std::enable_shared_from_this<Process>,
/// Returns \b true if the process successfully detaches, \b
/// false otherwise.
virtual Status DoDetach(bool keep_stopped) {
- Status error;
- error.SetErrorStringWithFormatv(
+ return Status::FromErrorStringWithFormatv(
"error: {0} does not support detaching from processes",
GetPluginName());
- return error;
}
/// Called after detaching from a process.
@@ -1228,11 +1212,9 @@ class Process : public std::enable_shared_from_this<Process>,
/// \return
/// Returns an error object.
virtual Status DoSignal(int signal) {
- Status error;
- error.SetErrorStringWithFormatv(
+ return Status::FromErrorStringWithFormatv(
"error: {0} does not support sending signals to processes",
GetPluginName());
- return error;
}
virtual Status WillDestroy() { return Status(); }
@@ -1686,7 +1668,7 @@ class Process : public std::enable_shared_from_this<Process>,
/// The number of bytes that were actually written.
virtual size_t DoWriteMemory(lldb::addr_t vm_addr, const void *buf,
size_t size, Status &error) {
- error.SetErrorStringWithFormatv(
+ error = Status::FromErrorStringWithFormatv(
"error: {0} does not support writing to processes", GetPluginName());
return 0;
}
@@ -1769,7 +1751,7 @@ class Process : public std::enable_shared_from_this<Process>,
virtual lldb::addr_t DoAllocateMemory(size_t size, uint32_t permissions,
Status &error) {
- error.SetErrorStringWithFormatv(
+ error = Status::FromErrorStringWithFormatv(
"error: {0} does not support allocating in the debug process",
GetPluginName());
return LLDB_INVALID_ADDRESS;
@@ -2036,11 +2018,9 @@ class Process : public std::enable_shared_from_this<Process>,
/// \return
/// \b true if the memory was deallocated, \b false otherwise.
virtual Status DoDeallocateMemory(lldb::addr_t ptr) {
- Status error;
- error.SetErrorStringWithFormatv(
+ return Status::FromErrorStringWithFormatv(
"error: {0} does not support deallocating in the debug process",
GetPluginName());
- return error;
}
/// The public interface to deallocating memory in the process.
@@ -2133,7 +2113,7 @@ class Process : public std::enable_shared_from_this<Process>,
/// less than \a buf_size, another call to this function should
/// be made to write the rest of the data.
virtual size_t PutSTDIN(const char *buf, size_t buf_size, Status &error) {
- error.SetErrorString("stdin unsupported");
+ error = Status::FromErrorString("stdin unsupported");
return 0;
}
@@ -2156,17 +2136,13 @@ class Process : public std::enable_shared_from_this<Process>,
size_t GetSoftwareBreakpointTrapOpcode(BreakpointSite *bp_site);
virtual Status EnableBreakpointSite(BreakpointSite *bp_site) {
- Status error;
- error.SetErrorStringW...
[truncated]
|
lldb/test/API/functionalities/scripted_process/invalid_scripted_process.py
Outdated
Show resolved
Hide resolved
clayborg
reviewed
Aug 27, 2024
There was a problem hiding this comment.
I worry the assert will kill the LLDB library and cause issues. Can we add a temporary setting that can override this in case it does cause crashes? I really don't want LLDB crashing if we can help it. It will be hard to test all of the error code paths that can happen out in the wild
Michael137
reviewed
Aug 27, 2024
|
I like this. I have just two remarks:
|
@clayborg When I say assert I had in mind something like this: So it would only "crash" in an asserts build, but otherwise keep working in production. Does that address you concern? |
From previous experience, I'm convinced that I'm going to break some platform-specific bots with this commit. I like the idea of only reverting the commit that removes the old API while that process is ongoing!
I picked these names, because they are in line with the old names, which made the regex replacement feasible. Renaming them afterwards is going to be easier. |
adrian-prantl
force-pushed
the
status-valuetype
branch
from
7917b74 to
37febe7
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
adrian-prantl
force-pushed
the
status-valuetype
branch
2 times, most recently
from
000d1cf to
016a797
Compare
|
On Aug 27, 2024, at 8:51 AM, Adrian Prantl ***@***.***> wrote:
I like this. I have just two remarks:
it might be better to split this into three steps (add new APIs, port to new APIs, remove old APIs), as that will make reverts easier/less disruptive (I don't know how much we can trust pre-commit CI these days, but I wouldn't be surprised if this breaks some platform-specific code).
From previous experience, I'm convinced that I'm going to break some platform-specific bots with this commit. I like the idea of only reverting the commit that removes the old API while that process is ongoing!
since this seems like a perfect opportunity to bikesh^Wdiscuss the names, I'm going to ask if there's any appetite for shortening some of the new factory functions. Status::FromErrorStringWithFormatv is a bit of a mouthful, so I was thinking if we could use something shorter instead (Status::FromFormatv or even Status::Formatv) ?
I picked these names, because they are in line with the old names, which made the regex replacement feasible. Renaming them afterwards is going to be easier.
My 2 cents on the naming: I had Status::FromFormatv in a previous iteration of this patch and changed my mind, because it doesn't indicate that this is going to be an error. What do you think about the slightly shorter Status::ErrorFromFromatv()?
Or, more radical, and potentially really confusing with LLVM code: rename lldb_private::Status to lldb_private::Error. I don't think I'd like that.
Note that lldb::Status WAS lldb::Error for a very long time till LLVM added its Error classes, and renamed the lldb ones in doing so. That's why you see
Status error;
everywhere in lldb. So going back is unlikely to find favor...
Jim
… —
Reply to this email directly, view it on GitHub <#106163 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWYUU3EX3LF7GGFUCJ3ZTSOAPAVCNFSM6AAAAABNFAYGXKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJSHEZTKOBYGQ>.
You are receiving this because you are on a team that was mentioned.
|
|
That should of course be lldb_private::Error -> lldb_private::Status...
Jim
… On Aug 27, 2024, at 9:48 AM, Jim Ingham ***@***.***> wrote:
>
>
>
>> On Aug 27, 2024, at 8:51 AM, Adrian Prantl ***@***.***> wrote:
>>
>>
>> I like this. I have just two remarks:
>>
>> it might be better to split this into three steps (add new APIs, port to new APIs, remove old APIs), as that will make reverts easier/less disruptive (I don't know how much we can trust pre-commit CI these days, but I wouldn't be surprised if this breaks some platform-specific code).
>> From previous experience, I'm convinced that I'm going to break some platform-specific bots with this commit. I like the idea of only reverting the commit that removes the old API while that process is ongoing!
>>
>> since this seems like a perfect opportunity to bikesh^Wdiscuss the names, I'm going to ask if there's any appetite for shortening some of the new factory functions. Status::FromErrorStringWithFormatv is a bit of a mouthful, so I was thinking if we could use something shorter instead (Status::FromFormatv or even Status::Formatv) ?
>> I picked these names, because they are in line with the old names, which made the regex replacement feasible. Renaming them afterwards is going to be easier.
>> My 2 cents on the naming: I had Status::FromFormatv in a previous iteration of this patch and changed my mind, because it doesn't indicate that this is going to be an error. What do you think about the slightly shorter Status::ErrorFromFromatv()?
>> Or, more radical, and potentially really confusing with LLVM code: rename lldb_private::Status to lldb_private::Error. I don't think I'd like that.
>>
>
> Note that lldb::Status WAS lldb::Error for a very long time till LLVM added its Error classes, and renamed the lldb ones in doing so. That's why you see
>
> Status error;
>
> everywhere in lldb. So going back is unlikely to find favor...
>
> Jim
>
>> —
>> Reply to this email directly, view it on GitHub <#106163 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWYUU3EX3LF7GGFUCJ3ZTSOAPAVCNFSM6AAAAABNFAYGXKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJSHEZTKOBYGQ>.
>> You are receiving this because you are on a team that was mentioned.
>>
>
>
|
adrian-prantl
force-pushed
the
status-valuetype
branch
3 times, most recently
from
f24793d to
afd926c
Compare
|
Updated all the non-Darwin plugins. I believe to have addressed all outstanding comments now. |
bulbazord
approved these changes
Aug 27, 2024
|
My concerns are addressed, thanks @adrian-prantl |
Michael137
reviewed
Aug 27, 2024
JDevlieghere
approved these changes
Aug 27, 2024
adrian-prantl
force-pushed
the
status-valuetype
branch
2 times, most recently
from
8c7cc31 to
d9e2226
Compare
This patch removes all of the Set.* methods from Status.
This cleanup is part of a series of patches that make it harder use
the anti-pattern of keeping a long-lives Status object around and
updating it while dropping any errors it contains on the floor.
This patch is largely NFC, the more interesting next steps this enables is to:
1. remove Status.Clear()
2. assert that Status::operator=() never overwrites an error
3. remove Status::operator=()
Note that step (2) will bring 90% of the benefits for users, and step
(3) will dramatically clean up the error handling code in various
places. In the end my goal is to convert all APIs that are of the form
ResultTy DoFoo(Status& error)
to
llvm::Expected<ResultTy> DoFoo()
How to read this patch?
The interesting changes are in Status.h and Status.cpp,
all other changes are mostly
perl -pi -e 's/\.SetErrorString/ = Status::FromErrorString/g' $(git grep -l SetErrorString lldb/source)
plus the occasional manual cleanup.
adrian-prantl
force-pushed
the
status-valuetype
branch
from
d9e2226 to
8e0de52
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/6005 Here is the relevant piece of the build log for the reference |
I think it's better (because its shorter), though it's still a bit long for my taste. FWIW, I don't find the short name at all confusing, because my mental model of these error types is (and all types I know behave that way) is that they have only one "success" value, and everything else represents some kind of an error/failure. So, when I see anything more complicated than I guess one difference is that I'm used to working with |
|
Thankfully this time around the renaming can actually be handled by |
adrian-prantl
added a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Oct 11, 2024
This patch removes all of the Set.* methods from Status. This cleanup is part of a series of patches that make it harder use the anti-pattern of keeping a long-lives Status object around and updating it while dropping any errors it contains on the floor. This patch is largely NFC, the more interesting next steps this enables is to: 1. remove Status.Clear() 2. assert that Status::operator=() never overwrites an error 3. remove Status::operator=() Note that step (2) will bring 90% of the benefits for users, and step (3) will dramatically clean up the error handling code in various places. In the end my goal is to convert all APIs that are of the form ` ResultTy DoFoo(Status& error) ` to ` llvm::Expected<ResultTy> DoFoo() ` How to read this patch? The interesting changes are in Status.h and Status.cpp, all other changes are mostly ` perl -pi -e 's/\.SetErrorString/ = Status::FromErrorString/g' $(git grep -l SetErrorString lldb/source) ` plus the occasional manual cleanup. (cherry picked from commit 0642cd7)
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.
This patch removes all of the Set.* methods from Status.
This cleanup is part of a series of patches that make it harder use the anti-pattern of keeping a long-lives Status object around and updating it while dropping any errors it contains on the floor.
This patch is largely NFC, the more interesting next steps this enables is to:
Note that step (2) will bring 90% of the benefits for users, and step (3) will dramatically clean up the error handling code in various places. In the end my goal is to convert all APIs that are of the form
ResultTy DoFoo(Status& error)to
llvm::Expected<ResultTy> DoFoo()How to read this patch?
The interesting changes are in Status.h and Status.cpp, all other changes are mostly
perl -pi -e 's/\.SetErrorString/ = Status::FromErrorString/g' $(git grep -l SetErrorString lldb/source)plus the occasional manual cleanup.