[Lldb-commits] [PATCH] D13058: LLDB-MI: Bug when evaluating strings containing characters from non-ascii range

[Enrico Granata via lldb-commits]

granata.enrico requested changes to this revision.
granata.enrico added a comment.
This revision now requires changes to proceed.

Sorry, I didn't realize you were still waiting on me to land this patch.

I have a few extra comments before this is ready to land.


================
Comment at: include/lldb/API/SBTypeSummary.h:125
@@ +124,3 @@
+
+        bool DoesPrintValue(const lldb::SBValue &value) const;
+
----------------
Can you please change this to

bool
DoesPrintValue (lldb::SBValue value);

There is no need to pass the SBValue by reference since you're not actually altering it - and since SBValue wraps a (glorified) pointer to a ValueObject you're not really copying much data over. I don't see the need to classify the function as const (only IsValid() seems to be marked thusly in that file), and definitely no need for the SBValue to be a const

================
Comment at: source/API/SBTypeSummary.cpp:289
@@ -286,1 +288,3 @@
 
+bool
+SBTypeSummary::DoesPrintValue(const lldb::SBValue &value) const
----------------
Of course change this in the C++ file as well

================
Comment at: tools/lldb-mi/MICmnLLDBUtilSBValue.cpp:372
@@ +371,3 @@
+CMIUtilString
+CMICmnLLDBUtilSBValue::GetValueSummary() const
+{
----------------
I might be missing something, but how is this going to work when an SBValue has a value but no summary (something as simple as int x = 1;)?

GetSummary() is free to return NULL, and I only see you fetching the value here when there is a summary. Your logic should be more like

GetSummary()
if (!summary || summary && DoesPrintValue())
 append value

A very long-term goal of mine is for all of this logic to exist in the ValueObjectPrinter and clients to simply be able to ask it to print a ValueObject according to some specification of theirs, in the middle of an existing stream. But that is way beyond the scope of your patch, so if you just clean up the logic here to do the right thing for values without summaries, I am happy.


http://reviews.llvm.org/D13058