[Lldb-commits] [PATCH] Fix the variable printing for stack-list-local and stack-list-arguments.

[Hafiz Abid Qadeer]

Please see my comments inline. Also it will be good if you can check that test pass on OSX.

In http://reviews.llvm.org/D7589#122628, @ki.stfu wrote:

> > RE: TODO: Those hardcoded line numbers should be removed.
>
>
> I think we should leave them to check *stopped notification. We can move it to separate test file to avoid modification of it when we are improving the tests.


Yes that will not be part of this revision. Just wanted to draw the attention. I was thinking that we can use the line_number () like function to get the line to check instead of hard coding them. But it can be done later.


================
Comment at: tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp:729
@@ +728,3 @@
+                    CMIUtilString valueStr = miValueList.ExtractContentNoBrackets();
+                    valueStr = valueStr.Trim('\"');
+                    // Surround by {} if there is , inside indicating multiple values.
----------------
ki.stfu wrote:
> Is it enough? Should we unescape this string?
I will check.

================
Comment at: tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp:733-734
@@ +732,4 @@
+                    {
+                        valueStr = "{" + valueStr;
+                        valueStr += "}";
+                    }
----------------
ki.stfu wrote:
> valueStr = CMIUtilString::Format("{%s}", valueStr.c_str());
I will change it. I dont like the unnecessary string copying that goes on in lldb-mi. Something to fix in future.

================
Comment at: tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp:738
@@ +737,3 @@
+                    const CMICmnMIValueResult miValueResult2("value", miValueConst2);
+                    miValueTuple.Add(miValueResult); // name
+                    miValueTuple.Add(miValueResult2);
----------------
ki.stfu wrote:
> Maybe would better to name it like miValueResultName? I know that lldb-mi uses sequential naming standard for miValueConst/miValueResult values, but in your case it was created on 20 lines above.
ok. I will change it. You may have already guesssed but it is done to have output like
[name="var"] when there is no value field. 

If it is added in the tuple first then we will get
[{name="var"}]
which I dont think is wrong but I wanted to be as close to what gdb does.

================
Comment at: tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp:766
@@ -775,4 +765,3 @@
 CMICmnLLDBDebugSessionInfo::GetVariableInfo(const MIuint vnMaxDepth, const lldb::SBValue &vrValue, const bool vbIsChildValue,
-                                            const VariableInfoFormat_e veVarInfoFormat, CMICmnMIValueList &vwrMiValueList,
-                                            MIuint &vrwnDepth)
+                                            CMICmnMIValueList &vwrMiValueList, MIuint &vrwnDepth)
 {
----------------
ki.stfu wrote:
> vrwnDepth must be a reference? I think it can cause a problem on large objects. For example, on line #844 you increment it in cycle (which serves to obtain a value of complex object). But on every iteration we will call GetVariableInfo with vrwnDepth which grows, but actually these children have one depth on call stack.
You are right. I think this should be decremented after function return to have the right depth counter. Current implementation will fail if the child struct have more then 10 members.

http://reviews.llvm.org/D7589

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/