[Lldb-commits] [PATCH] D35615: Add data formatter for libc++ std::tuple

[Jim Ingham via Phabricator via lldb-commits]

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

It seems awkward to me to be returning empty ValueObjectSP's by returning nullptr and then relying on conversion.  We don't do it that way in most of the other formatters.  If there wasn't a strong reason for doing it this way I think it would be clearer to return the object you're supposed to return.

Other than that this looks fine.



================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp:47
+  m_elements.assign(m_base_sp->GetCompilerType().GetNumDirectBaseClasses(),
+                    nullptr);
+  return false;
----------------
nullptr -> ValueObjectSP.  It makes it clearer reading the thing what it is.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp:53
+  if (idx >= m_elements.size())
+    return nullptr;
+  if (!m_base_sp)
----------------
It looks a little weird to return nullptr for a function returning a ValueObjectSP.  It would be clearer to return ValueObjectSP(), and that's what we do pretty much everywhere else.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp:55
+  if (!m_base_sp)
+    return nullptr;
+  if (m_elements[idx])
----------------
ditto


https://reviews.llvm.org/D35615