[lldb] Assert that CommandResultObject error messages are not empty

[DavidSpickett]

Summary

The intention is now that AppendError/SetError/AppendRawError only
be called with some message to show. This enforces that.

For SetError with a Status and a fallback string first assert
that the Status is a failure Status. Then it calls SetError(StringRef)
which checks the message is valid. (which could be the fallback
or the Status')

[dblaikie]

Sounds good, if this already passes all tests/etc (ie: there actually aren't any callers passing empty strings/non-failed errors/etc)

lldb/source/Interpreter/CommandReturnObject.cpp

122

I'd prefer to write that as !in_string.empty() - it's a somewhat more direct statement of the intent (& good habit to use empty on containers rather than size != 0 - some containers can have more expensive size operations).

Similarly above.

[DavidSpickett]

Use !empty instead of size. All tests passing on X86 and AArch64.

[teemperor]

LGTM, thanks for cleaning this up.

If we're anyway signing up @DavidSpickett for refactoring duty: CommandReturnObject::SetError(llvm::StringRef error_str) is clearly the same method as CommandReturnObject::AppendError, so one of those probably should go the way of the dodo. (Mostly kidding here, you don't have to refactor this too unless you have some time to spare)

[DavidSpickett]

Yeah good point, I can do that. I'm gonna go over the remaining SetStatus calls first, then I'll look at it.

[teemperor]

In D104525#2830025, @DavidSpickett wrote:

Yeah good point, I can do that. I'm gonna go over the remaining SetStatus calls first, then I'll look at it.

Thank you, really appreciated!

[DavidSpickett]

This comment was removed by DavidSpickett.

lldb/source/Interpreter/CommandReturnObject.cpp

 //===-- CommandReturnObject.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/CommandReturnObject.h"
 
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 static llvm::raw_ostream &error(Stream &strm) {

 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);
-  if (in_string.empty())
-    return;
+  assert(in_string.size() && "Expected a non-empty error message");
   error(GetErrorStream()) << in_string.rtrim() << '\n';
 }
 
 void CommandReturnObject::SetError(const Status &error,
                                    const char *fallback_error_cstr) {
-  const char *error_cstr = error.AsCString();
-  if (error_cstr == nullptr)
-    error_cstr = fallback_error_cstr;
-  SetError(error_cstr);
+  assert(error.Fail() && "Expected a failed Status");
+  SetError(error.AsCString(fallback_error_cstr));
 }
 
 void CommandReturnObject::SetError(llvm::StringRef error_str) {
   SetStatus(eReturnStatusFailed);
-  if (error_str.empty())
-    return;
-
   AppendError(error_str);
 }
 
 // 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);
-  if (in_string.empty())
-    return;
+  assert(in_string.size() && "Expected a non-empty error message");
   GetErrorStream() << in_string;
 }
 
 void CommandReturnObject::SetStatus(ReturnStatus status) { m_status = status; }
 
 ReturnStatus CommandReturnObject::GetStatus() { return m_status; }
 
 bool CommandReturnObject::Succeeded() {