[Lldb-commits] [PATCH] D104380: [lldb] Set return object failed status even if error string is empty

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D104380#2826465 <https://reviews.llvm.org/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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104380