[Lldb-commits] [PATCH] D104778: [lldb] Remove asserts in CommandReturnObject SetError and AppendError

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D104778#2835701 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104778/new/

https://reviews.llvm.org/D104778