Page MenuHomePhabricator

LLDB-MI: Fix assignment operator in CMIUtilString
ClosedPublic

Authored by evgeny777 on Sep 23 2015, 5:25 AM.

Details

Summary

The *this == vpRhs verification doesn't make any sense at all. It is a slows down and also will crash debugger if vpRhs is NULL, despite the check two lines below.

Would like this to be fixed, because it is quite easy to get SEGFAULT with peace of code like this

SBValue val;
CMIUtilString s;
//...
s = val.GetSummary();

Diff Detail

Repository
rL LLVM

Event Timeline

evgeny777 updated this revision to Diff 35481.Sep 23 2015, 5:25 AM
evgeny777 retitled this revision from to LLDB-MI: Fix assignment operator in CMIUtilString.
evgeny777 updated this object.
evgeny777 added reviewers: ki.stfu, clayborg.
evgeny777 added subscribers: dawn, KLapshin.

Not sure if this matters, but "Throws: None" is misleading. std::string::assign can throw std::length_error and std::bad_alloc

evgeny777 edited subscribers, added: lldb-commits; removed: dawn.Sep 23 2015, 5:43 AM
clayborg accepted this revision.Sep 23 2015, 10:39 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Sep 23 2015, 10:39 AM
ki.stfu requested changes to this revision.Sep 23 2015, 11:19 PM
ki.stfu edited edge metadata.

Your fix in assign operator looks good. Others go out of scope of this CL, so please revert them.

tools/lldb-mi/MIUtilString.cpp
41 ↗(On Diff #35481)

Not sure about usefulness of these changes. The NULL usually means an error in contrast to "" which means "there is nothing to show". In your case, you can check whether it's NULL or not, and then construct an object.

Besides that, you can use operator=:

SBValue val;
CMIUtilString s;
if (const char* s_cstr = val.GetSummary())
  s = s_cstr;
45–56 ↗(On Diff #35481)

Is it really used somewhere?

This revision now requires changes to proceed.Sep 23 2015, 11:19 PM

Without this change I have to use temporary const char* variable each time I call GetData() GetSummary(), GetValue() and so on
It would be nice to be able to do something like this

CMIUtilString s = v.GetSummary();
if (!s.empty()) {

CMIUtilString v = v.GetValue();
if (!v.empty()) {
     // and so on
}

}

And whenever I need error check I can use temporary const char* or just "if (v.GetSummary() != NULL)

Also in MI empty value and NULL value almost always mean "no output". Checking for NULL everywhere is just not convenient

evgeny777 added inline comments.Sep 24 2015, 3:32 AM
tools/lldb-mi/MIUtilString.cpp
41 ↗(On Diff #35481)

As I already said, in MI both NULL and "" mean nothing to output. Regarding your proposal compare this

CMIUtilString s;
if (const char* ss = val.GetSummary())  {
    if ((s=ss).size()) {
    }

}

with this

if ( (s = val.GetSummary()).size() ) {
}
brucem added a subscriber: brucem.Sep 24 2015, 5:29 AM

It doesn't seem right to have a strong construction and copy just to determine the length or if it is empty or not as in your examples. That would be appropriate for a helper function.

evgeny777 updated this revision to Diff 35620.Sep 24 2015, 7:10 AM
evgeny777 edited edge metadata.

Revised, now contains fix for just assignment operation
The "if(vpRhs != nullptr)" was removed to provide the same behaviour for

a) CMIUtilString s = (char*)nullptr;
b) s = (char*)nullptr;

ki.stfu accepted this revision.Sep 25 2015, 12:04 AM
ki.stfu edited edge metadata.
This revision is now accepted and ready to land.Sep 25 2015, 12:04 AM
This revision was automatically updated to reflect the committed changes.