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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
