Page MenuHomePhabricator

[lldb] Assert that CommandResultObject error messages are not empty
ClosedPublic

Authored by DavidSpickett on Jun 18 2021, 5:30 AM.

Details

Summary

The intention is now that AppendError/SetError/AppendRawError only
be called with some message to show. This enforces that.

For SetError with a Status and a fallback string first assert
that the Status is a failure Status. Then it calls SetError(StringRef)
which checks the message is valid. (which could be the fallback
or the Status')

Diff Detail

Event Timeline

DavidSpickett created this revision.Jun 18 2021, 5:30 AM
DavidSpickett requested review of this revision.Jun 18 2021, 5:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2021, 5:30 AM
dblaikie accepted this revision.Jun 18 2021, 11:38 AM

Sounds good, if this already passes all tests/etc (ie: there actually aren't any callers passing empty strings/non-failed errors/etc)

lldb/source/Interpreter/CommandReturnObject.cpp
122

I'd prefer to write that as !in_string.empty() - it's a somewhat more direct statement of the intent (& good habit to use empty on containers rather than size != 0 - some containers can have more expensive size operations).

Similarly above.

This revision is now accepted and ready to land.Jun 18 2021, 11:38 AM

Use !empty instead of size. All tests passing on X86 and AArch64.

DavidSpickett marked an inline comment as done.Jun 21 2021, 2:43 AM
This revision was landed with ongoing or failed builds.Jun 21 2021, 2:44 AM
This revision was automatically updated to reflect the committed changes.

LGTM, thanks for cleaning this up.

If we're anyway signing up @DavidSpickett for refactoring duty: CommandReturnObject::SetError(llvm::StringRef error_str) is clearly the same method as CommandReturnObject::AppendError, so one of those probably should go the way of the dodo. (Mostly kidding here, you don't have to refactor this too unless you have some time to spare)

Yeah good point, I can do that. I'm gonna go over the remaining SetStatus calls first, then I'll look at it.

Yeah good point, I can do that. I'm gonna go over the remaining SetStatus calls first, then I'll look at it.

Thank you, really appreciated!

This comment was removed by DavidSpickett.