LLDB-MI: Fix assignment operator in CMIUtilString

[evgeny777]

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();

[evgeny777]

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

[clayborg]

Looks good.

[ki.stfu]

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?

[evgeny777]

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)

[evgeny777]

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

[evgeny777]

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]

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]

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;

lldb/trunk/tools/lldb-mi/MIUtilString.cpp

 // Details: CMIUtilString assignment operator.
 // Type:    Method.
 // Args:    vpRhs   - Pointer to UTF8 text data.
 // Return:  CMIUtilString & - *this string.
 // Throws:  None.
 //--
 CMIUtilString &CMIUtilString::operator=(const char *vpRhs)
 {
-    if (*this == vpRhs)
-        return *this;
-
-    if (vpRhs != nullptr)
-    {
-        assign(vpRhs);
-    }
-
     return *this;
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: CMIUtilString assignment operator.
 // Type:    Method.
 // Args:    vrRhs   - The other string to copy from.
 // Return:  CMIUtilString & - *this string.