[Lldb-commits] [PATCH] D113362: [formatters] Add a libstdcpp formatter for forward_list and refactor list formatter

[walter erquinigo via Phabricator via lldb-commits]

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

Much better. Just some better patterns that you could follow. Read my comments.

Besides that, now you can unify the tests across libcxx and libstdcpp



================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:13-15
+    def __init__(self, valobj, dict, has_prev, node_value_pointer_offset):
         logger = lldb.formatters.Logger.Logger()
         self.valobj = valobj
----------------
Add these comments and remove the node_value_pointer_offset, as it can be inferred from has_prev


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:18
+        self.has_prev = has_prev
+        self.node_value_pointer_offset = node_value_pointer_offset
         logger >> "Providing synthetic children for a list named " + \
----------------
remove


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:72
                 self.count = self.num_children_impl()
-        return self.count
+        return self.count 
 
----------------
remove this trailing space


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:85-91
+                prev_val = self.prev.GetValueAsUnsigned(0)
+                if prev_val == 0:
+                    return 0
+                if next_val == self.node_address:
+                    return 0
+                if next_val == prev_val:
+                    return 1               
----------------
nice


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:92
+                    return 1               
+            size = self.node_value_pointer_offset
             current = self.next
----------------
let's better initialize this with a simpler value


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:98-99
                 current = current.GetChildMemberWithName('_M_next')
-            return (size - 1)
+            if self.has_prev:
+                return (size-1)
+            return size
----------------
remove this or do any necessary corrections here


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:125-127
             return current.CreateChildAtOffset(
                 '[' + str(index) + ']',
+               self.node_value_pointer_offset * current.GetType().GetByteSize(),
----------------



================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:159-162
+    # This address is used to identify if a node traversal has reached its end
+    # and is mandatory to be overriden in each AbstractListSynthProvider child class
+    def get_end_of_list_address(self):
+        raise NotImplementedError
----------------
good


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:169-172
+        # Lists have 1 or more pointers followed by the value.
+        # We should specify the number of the pointers for the correct
+        # size determination
+        node_value_pointer_offset = 1
----------------
remove


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:175-182
+    def num_children(self):
+        return super().num_children()
+
+    def get_child_at_index(self, index):
+        return super().get_child_at_index(index)
+
+    def extract_type(self):
----------------
you can delete these


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:184-188
+    def update(self):
+        super().update()
+        self.node = self.impl.GetChildMemberWithName('_M_head')
+        self.next = self.node.GetChildMemberWithName('_M_next')
+        return False
----------------
instead of this, declare a method in the parent class
  def updateNodes(self)
    ... # don't return anything
that is invoked by 'update'. 
both children must implement this method. That way you don't need to invoke super. Also you should write a comment that the 'udpateNodes' method must give value to self.node and self.next, and optionally to self.prev


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:199-202
+        # Lists have 1 or more pointers followed by the value.
+        # We should specify the number of the pointers for the correct
+        # size determination
+        node_value_pointer_offset = 2
----------------
remove


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113362/new/

https://reviews.llvm.org/D113362