[Lldb-commits] [PATCH] D26124: [LLDB-MI] Escape MI output in a more consistent manner

[Ilia K via Phabricator via lldb-commits]

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

@enlight , thanks for giving a chance to review it, and sorry that it took to long from my side.

Most of these changes look good to me, especially removing of meaningless temporal variables during construction of Result values. But I can't agree with that:

> I've also removed the spacing code from CMICmnMIValueTuple and CMICmnMIValueResult, it was only misused to format composite values in CMICmnLLDBUtilSBValue::GetCompositeValue(). The format of composite values isn't specified by the GDB-MI spec, and as such these values shouldn't be built using CMICmnMIValueTuple, CMICmnMIValueResult, and CMICmnMIValueConst.

I'm not sure about extra spaces in `tuple` or `result` records (`vbUseSpacing` opt in `CMICmnMIValueResult` and `CMICmnMIValueTuple` respectively), but regarding `CMICmnLLDBUtilSBValue::GetCompositeValue` I'd like to leave "as is". Construction of tuples by hands (see my inline comment) looks terrible for me while using of `CMICmnMIValueTuple` looked very naturally.

Regarding your note about the format of composite values, the[[ https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Output-Syntax.html#GDB_002fMI-Output-Syntax | GDB-MI spec ]] clearly says that:

  result ==> variable "=" value
  variable ==> string 
  value ==> const | tuple | list 
  const ==> c-string 
  tuple ==> "{}" | "{" result ( "," result )* "}"

So, the aggregates are those whose root values are represented by tuples.



================
Comment at: tools/lldb-mi/MICmnLLDBUtilSBValue.cpp:197-208
+      if (vwrCompositeValue.back() != '{')
+        vwrCompositeValue += ", ";
+      vwrCompositeValue += CMIUtilString::Format("%s = %s",
+                                                 utilMember.GetName().c_str(),
+                                                 value.c_str());
     } else {
+      if (vwrCompositeValue.back() != '{')
----------------
here


Repository:
  rL LLVM

https://reviews.llvm.org/D26124