[lldb] Remove asserts in CommandReturnObject SetError and AppendError

[DavidSpickett]

Summary

I added asserts to these in https://reviews.llvm.org/D104525.
They are available (directly or otherwise) via the API so we
should not assert.

Restore the previous behaviour. If the message
is empty, we return early before printing anything.
For SetError don't assert that the error is a failure.

The remaining assert is in AppendRawError which
is not part of the API.

[teemperor]

LGTM, I'll add them in my patch. Thanks for the quick turnaround!

[DavidSpickett]

As noted, the assert in AppendRawError remains. It has two internal callers and no API definition.

[DavidSpickett]

LGTM, I'll add them in my patch. Thanks for the quick turnaround!

To clarify, is there still a regression with the API?

• This removes the asserts so you won't crash
• No functions are changed in lldb/include/lldb/API/SBCommandReturnObject.h

The one thing that is different is that calling the API SetError with an empty string will now set eReturnStatusFailed. So:

s = do_thing_and_return_err_string_if_err(...)
return_object.SetError(s)

Would now set it to failed even if s is empty.

Perhaps the SetError(...) functions should retain the empty message == nop behaviour and AppendError can set the status unconditionally?

[teemperor]

In D104778#2835701, @DavidSpickett wrote:

LGTM, I'll add them in my patch. Thanks for the quick turnaround!

To clarify, is there still a regression with the API?

No regression anymore from what I can see. My point was that I'll fix the usage from the SB API and then I can just re-add the asserts in the same patch (the asserts themselves are a good idea, the SB API just should have been updated to prevent hitting them from user scripts)

• This removes the asserts so you won't crash
• No functions are changed in lldb/include/lldb/API/SBCommandReturnObject.h

The one thing that is different is that calling the API SetError with an empty string will now set eReturnStatusFailed. So:

s = do_thing_and_return_err_string_if_err(...)
return_object.SetError(s)

Would now set it to failed even if s is empty.

Perhaps the SetError(...) functions should retain the empty message == nop behaviour and AppendError can set the status unconditionally?

I think the assert behaviour is fine for the internal API, but the SB API behaviour is a bit more tricky. But let's discuss the expected behaviour in a dedicated review for that.

lldb/source/Interpreter/CommandReturnObject.cpp

 void CommandReturnObject::AppendWarning(llvm::StringRef in_string) {
   if (in_string.empty())
     return;
   warning(GetErrorStream()) << in_string.rtrim() << '\n';
 }
 
 void CommandReturnObject::AppendError(llvm::StringRef in_string) {
   SetStatus(eReturnStatusFailed);
-  assert(!in_string.empty() && "Expected a non-empty error message");
+  if (in_string.empty())
+    return;
   error(GetErrorStream()) << in_string.rtrim() << '\n';
 }
 
 void CommandReturnObject::SetError(const Status &error,
                                    const char *fallback_error_cstr) {
-  assert(error.Fail() && "Expected a failed Status");
   AppendError(error.AsCString(fallback_error_cstr));
 }
 
 // Similar to AppendError, but do not prepend 'Status: ' to message, and don't
 // append "\n" to the end of it.
 
 void CommandReturnObject::AppendRawError(llvm::StringRef in_string) {
   SetStatus(eReturnStatusFailed);