This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Remove CommandReturnObject's SetError(StringRef)
ClosedPublic

Authored by DavidSpickett on Jun 23 2021, 2:03 AM.

Details

Summary

Replacing existing uses with AppendError.

SetError is also part of the SBI API. This remains
but instead of calling the underlying SetError it
will call AppendError.

Diff Detail

Event Timeline

DavidSpickett requested review of this revision.Jun 23 2021, 2:03 AM
DavidSpickett created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2021, 2:03 AM

I'm not totally sure about adding AppendError to the API.

Should we (and where should I) document that AppendError expects a non empty message? On the other hand if you're using a release build the assert to check that isn't a factor, you'll just set eReturnStatusFailed and not add any error text. So it's not going to come up for a lot of users.

Or I could just leave the API as is, with SetError calling AppendError internally.

teemperor requested changes to this revision.Jun 23 2021, 2:53 AM

We should never assert unless we have to within SB API calls as that takes down the whole debugging session and all this is directly available to users via the script interpreter. I didn't realize we exposed that function via the SB API when I looked at the callers of those functions in D104525, so that's a regression we should fix. I can make a patch for that.

Anyway, this LGTM but please drop the addition of AppendError from the SB API. Adding a permanent new function to the SB API requires a bit more review than NFC refactoring. So that's the way to go:

Or I could just leave the API as is, with SetError calling AppendError internally.

This revision now requires changes to proceed.Jun 23 2021, 2:53 AM

I was going to say we only would need to remove the assert for this one:

void CommandReturnObject::SetError(const Status &error,
                                   const char *fallback_error_cstr) {

Since SetError checks that the char * is not null in the API layer before calling SetError proper. However, the assert is that the string is not empty. You could pass in a non null ptr, to an empty string. So you're right, those need to be removed.

(I'll update this patch shortly)

Don't change the API, just have SetError call underlying
AppendError.

teemperor accepted this revision.Jun 23 2021, 4:11 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jun 23 2021, 4:11 AM
DavidSpickett edited the summary of this revision. (Show Details)Jun 23 2021, 4:23 AM
This revision was landed with ongoing or failed builds.Jun 23 2021, 4:25 AM
This revision was automatically updated to reflect the committed changes.