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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
