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