[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.

very good start!



================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:5-6
 
+FORWARD_LIST = "FORWARD_LIST"
+LIST = "LIST"
+
----------------
remove this


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:21-151
+    def next_node_abstract(self, node):
         logger = lldb.formatters.Logger.Logger()
         return node.GetChildMemberWithName('_M_next')
 
-    def is_valid(self, node):
+    def is_valid_abstract(self, node, break_condition):
         logger = lldb.formatters.Logger.Logger()
+        valid = self.value_abstract(self.next_node_abstract(node)) != break_condition
----------------
remove all these _abstract suffixes. They make reading harder


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:27
         logger = lldb.formatters.Logger.Logger()
-        valid = self.value(self.next_node(node)) != self.node_address
+        valid = self.value_abstract(self.next_node_abstract(node)) != break_condition
         if valid:
----------------
here you should call the method get_end_of_list_address that i mention below


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:41-42
 
-    # Floyd's cycle-finding algorithm
-    # try to detect if this list has a loop
-    def has_loop(self):
----------------
don't remove this command. It's useful to know what's going on


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:95
                 current = current.GetChildMemberWithName('_M_next')
-            return (size - 1)
+            return (size - size_correction)
         except:
----------------
what is this? try to come up with a better name with documentation to make this easier to understand


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:161
+    def num_children(self):
+        return super().num_children_abstract(incoming_size=1, size_correction=0, is_valid_break_condition=0, type=FORWARD_LIST)
+
----------------
don't pass the is_valid_break_condition variable everywhere. It makes code more confusing. Instead, you should create a method "get_end_of_list_address" that should be overridden by each child implementation. Then you add a comment that this address is used to identify if a node traversal has reached its end


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:161
+    def num_children(self):
+        return super().num_children_abstract(incoming_size=1, size_correction=0, is_valid_break_condition=0, type=FORWARD_LIST)
+
----------------
wallace wrote:
> don't pass the is_valid_break_condition variable everywhere. It makes code more confusing. Instead, you should create a method "get_end_of_list_address" that should be overridden by each child implementation. Then you add a comment that this address is used to identify if a node traversal has reached its end
don't pass 'type' here, instead, in the constructor of the parent in line 158 pass a boolean flag 'has_prev'. That should be enough for the parent class to know if there's a prev pointer or not


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:161
+    def num_children(self):
+        return super().num_children_abstract(incoming_size=1, size_correction=0, is_valid_break_condition=0, type=FORWARD_LIST)
+
----------------
wallace wrote:
> wallace wrote:
> > don't pass the is_valid_break_condition variable everywhere. It makes code more confusing. Instead, you should create a method "get_end_of_list_address" that should be overridden by each child implementation. Then you add a comment that this address is used to identify if a node traversal has reached its end
> don't pass 'type' here, instead, in the constructor of the parent in line 158 pass a boolean flag 'has_prev'. That should be enough for the parent class to know if there's a prev pointer or not
similarly, pass incoming_size to the constructor, and a better name is "node_value_pointer_offset", and you add a comment explaining that lists have 1 or more pointers followed by the value


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113362