[lldb] Set return object failed status even if error string is empty

[DavidSpickett]

Summary

The idea is now that AppendError<...> will set eReturnStatusFailed
for you so you don't have to call SetStatus again.

Previously if the error message was empty, the status wouldn't
be set.

I don't think there are any sitautions where the message is in
fact empty but it potentially could be depending on where
we get the string from.

So let's set the status up front then return early if the message is empty.

[teemperor]

At least on the test suite on Linux that branch is anyway never taken, so LGTM. Thanks!

[dblaikie]

This might be a case of an overly reduced patch - this patch alone I'd expect to be observable in some way, and tested. If the "instring.empty()" condition never fires, then I'd expect to change that to an assert, rather than changing the semantics of it in an unobservable way.

But I see there were follow-up NFC/refactoring commits that did take advantage of the new behavior - might've been more suitable to group the refactoring with this change in behavior, at least in one example of each of the 3 changes here (eg: for each one, change one caller and the implementation - then change the rest of the callers in separate follow-up commits if you prefer to break them out that way, to keep patches small in case they do introduce any regressions)

[DavidSpickett]

This change would have been part of https://reviews.llvm.org/D103701 if I had realised that this was in fact how it worked. I separated it from the follow ons because of that and to not bury 3 lines of change that apply to all callers of these functions, in 300 lines of change to callers of SetStatus(eReturnStatusFailed). If I were bisecting a failure caused by these changes I'd appreciate the separation but there's probably not much difference.

I agree about the assert, if you're not meant to pass empty strings we should enforce that. I'll get something into review.

[dblaikie]

In D104380#2826465, @DavidSpickett wrote:

This change would have been part of https://reviews.llvm.org/D103701 if I had realised that this was in fact how it worked. I separated it from the follow ons because of that and to not bury 3 lines of change that apply to all callers of these functions, in 300 lines of change to callers of SetStatus(eReturnStatusFailed). If I were bisecting a failure caused by these changes I'd appreciate the separation but there's probably not much difference.

Yeah, if it's a ton of call sites that are going to be updated - maybe this is simple enough that the interface semantic change + all the call sites being updated in one patch isn't the worst thing. But if it's more complicated I'd potentially do the interface semantic change/generalization + 1 caller to demonstrate the value, then the rest in subsequent cleanup commits.

Though sometimes I'll refactor an API (generalize it, split it into two functions, etc) without having changed any callers - perhaps because the new callers are coming in a subsequent more complicated patch.

No really hard rules here - just some thoughts. :)

I agree about the assert, if you're not meant to pass empty strings we should enforce that. I'll get something into review.

Thanks!

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;
-  SetStatus(eReturnStatusFailed);
   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);
 }
 
 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;
   GetErrorStream() << in_string;
-  SetStatus(eReturnStatusFailed);
 }
 
 void CommandReturnObject::SetStatus(ReturnStatus status) { m_status = status; }
 
 ReturnStatus CommandReturnObject::GetStatus() { return m_status; }
 
 bool CommandReturnObject::Succeeded() {
   return m_status <= eReturnStatusSuccessContinuingResult;