[Lldb-commits] [PATCH] D35556: Add data formatter for libc++'s forward_list

[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 looks like the behavior of the two derived list classes here are only partially factored out.  See the individual comments but it looks like there is a lot more that could be shared.

I also don't think we should change the naming conventions used for these classes piecemeal.  If we want to simplify the names and hide the FrontEnds in the source files we should do that wholesale, and not one by one.  Otherwise next time I read this file I'm going to waste time wondering why the ForwardListFrontEnd isn't specific to LibCxx whereas the LibcxxStdListSyntheticFrontEnd does seem to be...



================
Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/forward_list/main.cpp:5
+{
+  std::forward_list<int> empty{}, one_elt{47}, five_elts{1,22,333,4444,55555};
+  return 0; // break here
----------------
Some compilers (including clang at other times) will omit debug info for variables that it doesn't see getting used.  I usually try to use the variables I'm going to print (call size on them, pass an element to printf, whatever...) to keep this from happening.

OTOH, it's nice if compilers don't do that, so maybe relying on their not doing that in our tests is a useful forcing function???


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:141
 
-namespace lldb_private {
-namespace formatters {
-class LibcxxStdListSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
+class ForwardListFrontEnd: public AbstractListFrontEnd {
+public:
----------------
I wonder about the decision to move these classes out of lldb_private::formatters.  They don't need to be in a globally visible namespace, but all the others are.  They also all have a consistent naming convention, which this one doesn't have.  This doesn't seem like the sort of thing we should change piecemeal, that will just lead to confusion.

Was there some other reason for doing this that I'm missing?


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:153
+  std::map<size_t, ListIterator> m_iterators;
+};
+
----------------
Why are these three ivars not in the AbstractListFrontEnd? They appear in both derived classes and seem to be used in the same way.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:182
+  m_head = nullptr;
+  return false;
 }
----------------
The AbstractFrontEndList has the m_fast_runner and m_slow_runner but they are only reset in the Update for the LibcxxStdListSyntheticFrontEnd, and not here.  Is that on purpose?


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:251-260
+  ListIterator current(m_head);
+  if (idx > 0) {
+    auto cached_iterator = m_iterators.find(idx - 1);
+    if (cached_iterator != m_iterators.end()) {
+      current = cached_iterator->second;
+      actual_advance = 1;
+    }
----------------
This use of the iterators also seems like it should be shared.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:299-301
+  /*m_node_address = backend_addr->GetValueAsUnsigned(0);
+  if (!m_node_address || m_node_address == LLDB_INVALID_ADDRESS)
+    return false;*/
----------------
?


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:303-310
+  CompilerType list_type = m_backend.GetCompilerType();
+  if (list_type.IsReferenceType())
+    list_type = list_type.GetNonReferenceType();
+
+  if (list_type.GetNumTemplateArguments() == 0)
+    return false;
+  TemplateArgumentKind kind;
----------------
This bit is done in exactly the same way in the two sub-classes.  Could it be moved into the base class Update?


https://reviews.llvm.org/D35556