[Lldb-commits] [PATCH] D11488: Fix bug in expression display when determining if a pointer should be treated as a string

[Ilia K]

ki.stfu requested changes to this revision.
ki.stfu added a comment.
This revision now requires changes to proceed.

See my inline comments.


================
Comment at: test/tools/lldb-mi/variable/main.cpp:70-76
@@ -69,1 +69,9 @@
 
+struct not_str
+{
+    char c;
+    int f;
+    not_str(char _c, int _f) : c(_c), f(_f)
+    {}
+};
+
----------------
Use clang-format for formatting please.

================
Comment at: test/tools/lldb-mi/variable/main.cpp:83
@@ -74,2 +82,3 @@
     complex_type complx_array[2] = { { 4, { 4L }, &complx_array[1] }, { 5, { 5 }, &complx_array[0] } };
+    not_str* ps = new not_str('a', 0);
 
----------------
a memory leak. just allocate it on the stack and then use "&nstr"

================
Comment at: tools/lldb-mi/MICmnLLDBUtilSBValue.cpp:139-141
@@ -138,3 +138,5 @@
     {
-        if (m_bHandleCharType && IsFirstChildCharType())
+        const lldb::BasicType eType = m_rValue.GetType().GetPointeeType().GetBasicType();
+
+        if (m_bHandleCharType && IsCharBasicType(eType))
         {
----------------
Please wrap it into IsPointeeCharType():
```
        if (m_bHandleCharType && IsPointeeCharType())
```

================
Comment at: tools/lldb-mi/MICmnLLDBUtilSBValue.cpp:364
@@ -363,1 +363,3 @@
+// Details: Check that basic type is a char type. Char type can be signed or unsigned.
 // Type:    Method.
+// Args:    eType   - type to check
----------------
Change to "Static":
```
// Type:    Static.
```

================
Comment at: tools/lldb-mi/MICmnLLDBUtilSBValue.cpp:404
@@ -387,2 +403,2 @@
 //          a char type or some other type. Returns false if there are not children. Char
 //          type can be signed or unsigned.
----------------
Add IsPointeeCharType() function after IsFirstChildCharType():
```
//++ ------------------------------------------------------------------------------------
// Details: Retrieve the flag stating whether pointee object of *this object is a char
//          type or some other type. Returns false if there are not children. Char type
//          can be signed or unsigned.
// Type:    Method.
// Args:    None.          
// Return:  bool    - True = Yes is a char type, false = some other type.
// Throws:  None.
//--
bool
CMICmnLLDBUtilSBValue::IsPointeeCharType(void) const
{
    const MIuint nChildren = m_rValue.GetNumChildren();

    // Is it a basic type
    if (nChildren == 0)
        return false;

    const lldb::BasicType eType = m_rValue.GetType().GetPointeeType().GetBasicType();
    return IsCharBasicType(eType);
}
```

================
Comment at: tools/lldb-mi/MICmnLLDBUtilSBValue.h:57
@@ -57,2 +57,2 @@
     CMIUtilString GetSimpleValueCStringPointer(void) const;
     CMIUtilString GetSimpleValueCStringArray(void) const;
----------------
Add
```
bool IsPointeeCharType(void) const;
```

after these lines:
```
    CMIUtilString GetTypeName(void) const;
    CMIUtilString GetTypeNameDisplay(void) const;
    bool IsCharType(void) const;
    bool IsFirstChildCharType(void) const;
```

================
Comment at: tools/lldb-mi/MICmnLLDBUtilSBValue.h:60
@@ -59,2 +59,3 @@
     bool GetCompositeValue(const bool vbPrintFieldNames, CMICmnMIValueTuple &vwrMiValueTuple, const MIuint vnDepth = 1) const;
-
+    static bool IsCharBasicType(lldb::BasicType eType);
+    
----------------
Move to separate section:
```
    // Statics:
  private:
    static bool IsCharBasicType(lldb::BasicType eType);
```


http://reviews.llvm.org/D11488