Page MenuHomePhabricator

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

Authored by DavidSpickett on Jun 16 2021, 6:44 AM.

Details

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.

Diff Detail

Event Timeline

DavidSpickett requested review of this revision.Jun 16 2021, 6:44 AM
DavidSpickett created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2021, 6:44 AM
teemperor accepted this revision.Jun 16 2021, 8:24 AM

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

This revision is now accepted and ready to land.Jun 16 2021, 8:24 AM
This revision was landed with ongoing or failed builds.Jun 17 2021, 4:20 AM
This revision was automatically updated to reflect the committed changes.

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)

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.

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!