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

[Pavel Labath via Phabricator via lldb-commits]

labath marked 5 inline comments as done.
labath added inline comments.


================
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
----------------
jingham wrote:
> 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???
Hmm... I'd say we can leave it like this. I'd be surprised if the compiler can figure out that these declarations are a no-op without very aggressive inlining (which it just won't do at -O0).


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:141
 
-namespace lldb_private {
-namespace formatters {
-class LibcxxStdListSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
+class ForwardListFrontEnd: public AbstractListFrontEnd {
+public:
----------------
jingham wrote:
> 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?
There's no hard reason, but I was trying to make these names more concise

I've put them in the anonymous namespace, as llvm coding style encourages that for file-local entities <https://llvm.org/docs/CodingStandards.html#anonymous-namespaces>. I'd be happy to move the other formatters into the anonymous namespace as well, as I have a couple of other formatters in the pipeline, which already are in the anon namespace.

As for the names, if I followed the existing style, this would read something like
```
class LibCxxStdForwardListSyntheticFrontEnd: public LibCxxAbstractListSyntheticFrontEnd
```
which would be formatted horribly, as it does not fit a single line. As these are file-local entities, I think we can be a bit less verbose.

I had not renamed the std::list class because of the way I wrote this change (make a copy of the std::list formatter, and then factor out the common code), but you are right that they should follow the same convention. I propose to shorten the name of the other class as well.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:153
+  std::map<size_t, ListIterator> m_iterators;
+};
+
----------------
jingham wrote:
> Why are these three ivars not in the AbstractListFrontEnd? They appear in both derived classes and seem to be used in the same way.
Good question. I started with two completely distinct implementations and then worked towards merging them. I guess did not finish the job.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:182
+  m_head = nullptr;
+  return false;
 }
----------------
jingham wrote:
> 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?
It doesn't matter since they are re-initialized in HasLoop(), but it does look weird. Fixed it.


================
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;
+    }
----------------
jingham wrote:
> This use of the iterators also seems like it should be shared.
Indeed it can.


================
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;*/
----------------
jingham wrote:
> ?
Oops.


https://reviews.llvm.org/D35556